[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
dsanders marked an inline comment as done.
Closed by commit rL370512: [clang-tidy] Add llvm-prefer-register-over-unsigned 
to clang-tidy (authored by dsanders, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D65919?vs=218015=218156#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/trunk/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/trunk/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  
clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
  
clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm { };
+
+// This class shouldn't trigger it despite the similarity as it's not inside the llvm namespace
+class Register {
+public:
+  operator unsigned();
+};
+
+Register getReg();
+
+void do_nothing_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-FIXES: do_nothing_1()
+  // CHECK-FIXES-NEXT: unsigned Reg1 = getReg();
+}
+
+void do_nothing_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // CHECK-FIXES: do_nothing_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: unsigned Reg2 = getReg();
+}
+
+namespace llvm {
+void do_nothing_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-FIXES: do_nothing_3()
+  // CHECK-FIXES-NEXT: unsigned Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,143 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+
+// This class shouldn't trigger it despite the similarity.
+class RegisterLike {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+llvm::RegisterLike getRegLike();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // FIXME: Function-scoped UsingDirectiveDecl's don't appear to be in the
+  //DeclContext for the function so we can't elide the llvm:: in this
+  //case. Fortunately, it doesn't actually occur in the LLVM code base.
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: llvm::Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg1 = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void done_2() {
+  using namespace llvm;
+  Register Reg2 = getReg();
+  // CHECK-FIXES: done_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg2 = getReg();
+}
+
+namespace llvm {
+void done_3() {
+  Register Reg3 = getReg();
+  // CHECK-FIXES: done_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
+
+void do_nothing_1() {
+  unsigned Reg1 = getRegLike();
+  // CHECK-FIXES: do_nothing_1()
+  // CHECK-FIXES-NEXT: unsigned Reg1 = getRegLike();
+}
+
+void do_nothing_2() {
+  using 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders added a comment.

r370512


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a nit with `auto`.




Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:47
+for (const auto *UsingDirective : Context->using_directives()) {
+  const auto *Namespace = UsingDirective->getNominatedNamespace();
+  if (isa(Namespace->getDeclContext()) &&

Don't use `const auto *` here as the type is not spelled out in the initializer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 218015.
dsanders marked 4 inline comments as done.
dsanders added a comment.

- Sort order again :-)
- Avoid getQualifiedNameAsString() in favour of getName() and checking


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm { };
+
+// This class shouldn't trigger it despite the similarity as it's not inside the llvm namespace
+class Register {
+public:
+  operator unsigned();
+};
+
+Register getReg();
+
+void do_nothing_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-FIXES: do_nothing_1()
+  // CHECK-FIXES-NEXT: unsigned Reg1 = getReg();
+}
+
+void do_nothing_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // CHECK-FIXES: do_nothing_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: unsigned Reg2 = getReg();
+}
+
+namespace llvm {
+void do_nothing_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-FIXES: do_nothing_3()
+  // CHECK-FIXES-NEXT: unsigned Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+using namespace llvm;
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,143 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+
+// This class shouldn't trigger it despite the similarity.
+class RegisterLike {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+llvm::RegisterLike getRegLike();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // FIXME: Function-scoped UsingDirectiveDecl's don't appear to be in the
+  //DeclContext for the function so we can't elide the llvm:: in this
+  //case. Fortunately, it doesn't actually occur in the LLVM code base.
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: llvm::Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg1 = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void done_2() 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:32-33
+"llvm-prefer-register-over-unsigned");
+CheckFactories.registerCheck(
+"llvm-namespace-comment");
 CheckFactories.registerCheck("llvm-twine-local");

aaron.ballman wrote:
> Spurious change that moved `llvm-namespace-comment` to be out of alphabetical 
> order?
Oops, not sure what happened there. I think I rolled it back to one of the 
earlier revisions somehow


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Aside from the few remaining nits, I think this is almost ready to go.




Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:32-33
+"llvm-prefer-register-over-unsigned");
+CheckFactories.registerCheck(
+"llvm-namespace-comment");
 CheckFactories.registerCheck("llvm-twine-local");

Spurious change that moved `llvm-namespace-comment` to be out of alphabetical 
order?



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm

dsanders wrote:
> aaron.ballman wrote:
> > I'd also like to see some tests like:
> > ```
> > void func(unsigned Reg);
> > 
> > struct S {
> >   unsigned Reg;
> > 
> >   S(unsigned Reg) : Reg(Reg) {}
> > };
> > 
> > void other_func() {
> >   func(getReg());
> >   S s{getReg()};
> >   s.Reg = getReg();
> > }
> > ```
> Added tests for the following cases that do not make changes*
> 1. Similar interface but not called Register
> 2. Register class not inside llvm namespace
> 3. Calling a function that takes an llvm::Register with a llvm::Register 
> argument
> 4. Calling a function that takes an unsigned and is given an llvm::Register 
> argument
> 5. Initializing an llvm::Register using an llvm::Register argument
> 6. Initializing any other object using a constructor parameter that's 
> unsigned and a llvm::Register argument
> 7. Assigning to a member of llvm::Register
> 8. Assigning to a unsigned member of any other object
> 
> *4, 6, and 8 should eventually but don't yet
Nice, thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 216290.
dsanders marked 2 inline comments as done.
dsanders added a comment.

- registerCheck<> order


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm { };
+
+// This class shouldn't trigger it despite the similarity as it's not inside the llvm namespace
+class Register {
+public:
+  operator unsigned();
+};
+
+Register getReg();
+
+void do_nothing_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-FIXES: do_nothing_1()
+  // CHECK-FIXES-NEXT: unsigned Reg1 = getReg();
+}
+
+void do_nothing_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // CHECK-FIXES: do_nothing_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: unsigned Reg2 = getReg();
+}
+
+namespace llvm {
+void do_nothing_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-FIXES: do_nothing_3()
+  // CHECK-FIXES-NEXT: unsigned Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+using namespace llvm;
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,143 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+
+// This class shouldn't trigger it despite the similarity.
+class RegisterLike {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+llvm::RegisterLike getRegLike();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // FIXME: Function-scoped UsingDirectiveDecl's don't appear to be in the
+  //DeclContext for the function so we can't elide the llvm:: in this
+  //case. Fortunately, it doesn't actually occur in the LLVM code base.
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: llvm::Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg1 = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void done_2() {
+  using namespace llvm;
+  Register Reg2 = getReg();
+  // 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:26
   void addCheckFactories(ClangTidyCheckFactories ) override {
+using readability::NamespaceCommentCheck;
+

dsanders wrote:
> aaron.ballman wrote:
> > I would rather use the fully-qualified names below -- the namespaces are 
> > actually of interest when needing to see what checks rely on what other 
> > modules quickly.
> In that case I think I need some more detail on the alphabetical order I need 
> to preserve. Do the namespaces factor into the order? If so, then 
> PreferIsaOrDynCastInConditionalsCheck was in the wrong place prior to this 
> patch and add_new_check.py is behaving correctly. If not, then the existing 
> order was correct but add_new_check.py is inserting new lines incorrectly
We usually alphabetize based on the string literal for the check being 
registered. This usually results in the list also being sorted by the check 
implementation class name, but not always.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders marked an inline comment as done.
dsanders added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:26
   void addCheckFactories(ClangTidyCheckFactories ) override {
+using readability::NamespaceCommentCheck;
+

aaron.ballman wrote:
> I would rather use the fully-qualified names below -- the namespaces are 
> actually of interest when needing to see what checks rely on what other 
> modules quickly.
In that case I think I need some more detail on the alphabetical order I need 
to preserve. Do the namespaces factor into the order? If so, then 
PreferIsaOrDynCastInConditionalsCheck was in the wrong place prior to this 
patch and add_new_check.py is behaving correctly. If not, then the existing 
order was correct but add_new_check.py is inserting new lines incorrectly


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:26
   void addCheckFactories(ClangTidyCheckFactories ) override {
+using readability::NamespaceCommentCheck;
+

I would rather use the fully-qualified names below -- the namespaces are 
actually of interest when needing to see what checks rely on what other modules 
quickly.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:45
+if (const auto *Namespace = dyn_cast(Context))
+  if (Namespace->getQualifiedNameAsString() == "llvm")
+Replacement = "Register";

dsanders wrote:
> aaron.ballman wrote:
> > This is a fairly expensive operation compared to calling `getName()`; are 
> > there situations where you think you need the extra work to be done? (Same 
> > comment below.)
> None that are likely to occur. `getName()` with some way to check that 
> there's no enclosing namespace would be equivalent. Is there a cheap way to 
> tell if a namespace is at the top level?
You could check if its `DeclContext` is the translation unit, I believe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 214744.
dsanders marked 9 inline comments as done.
dsanders added a comment.

- Correct alphabetical list in LLVMTidyModule.cpp and prevent add_new_check.py 
getting it wrong in future
- Lookup Register class via ::llvm::Register instead of llvm::Register
- Removed unnecessary bindings
- Typo in the docs
- More tests

Haven't made the change to avoid getQualifiedNameAsString() yet in case there's 
an easy way to tell
if the llvm namespace found is inside another namespace.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm { };
+
+// This class shouldn't trigger it despite the similarity as it's not inside the llvm namespace
+class Register {
+public:
+  operator unsigned();
+};
+
+Register getReg();
+
+void do_nothing_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-FIXES: do_nothing_1()
+  // CHECK-FIXES-NEXT: unsigned Reg1 = getReg();
+}
+
+void do_nothing_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // CHECK-FIXES: do_nothing_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: unsigned Reg2 = getReg();
+}
+
+namespace llvm {
+void do_nothing_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-FIXES: do_nothing_3()
+  // CHECK-FIXES-NEXT: unsigned Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+using namespace llvm;
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,143 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+
+// This class shouldn't trigger it despite the similarity.
+class RegisterLike {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+llvm::RegisterLike getRegLike();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // FIXME: Function-scoped UsingDirectiveDecl's don't appear to be in the
+  //DeclContext for the function so we can't elide the llvm:: in this
+  //case. Fortunately, it doesn't actually occur in the LLVM code base.
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: llvm::Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:28-29
 CheckFactories.registerCheck("llvm-include-order");
+CheckFactories.registerCheck(
+"llvm-prefer-register-over-unsigned");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> Please keep this list sorted in alphabetical order.
This change was made by add_new_check.py and looking at the code, it's been 
confused by the `readability::`. I'll move it to the right place and add a 
`using readability::NamespaceCommentCheck` so we can drop the `readability::` 
and have add_new_check.py get it right in future.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:45
+if (const auto *Namespace = dyn_cast(Context))
+  if (Namespace->getQualifiedNameAsString() == "llvm")
+Replacement = "Register";

aaron.ballman wrote:
> This is a fairly expensive operation compared to calling `getName()`; are 
> there situations where you think you need the extra work to be done? (Same 
> comment below.)
None that are likely to occur. `getName()` with some way to check that there's 
no enclosing namespace would be equivalent. Is there a cheap way to tell if a 
namespace is at the top level?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst:10
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+

aaron.ballman wrote:
> initialize -> initialization
It was meant to be 'initializer'. I'll correct that



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm

aaron.ballman wrote:
> I'd also like to see some tests like:
> ```
> void func(unsigned Reg);
> 
> struct S {
>   unsigned Reg;
> 
>   S(unsigned Reg) : Reg(Reg) {}
> };
> 
> void other_func() {
>   func(getReg());
>   S s{getReg()};
>   s.Reg = getReg();
> }
> ```
Added tests for the following cases that do not make changes*
1. Similar interface but not called Register
2. Register class not inside llvm namespace
3. Calling a function that takes an llvm::Register with a llvm::Register 
argument
4. Calling a function that takes an unsigned and is given an llvm::Register 
argument
5. Initializing an llvm::Register using an llvm::Register argument
6. Initializing any other object using a constructor parameter that's unsigned 
and a llvm::Register argument
7. Assigning to a member of llvm::Register
8. Assigning to a unsigned member of any other object

*4, 6, and 8 should eventually but don't yet


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:28-29
 CheckFactories.registerCheck("llvm-include-order");
+CheckFactories.registerCheck(
+"llvm-prefer-register-over-unsigned");
 CheckFactories.registerCheck(

Please keep this list sorted in alphabetical order.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+  << Replacement

dsanders wrote:
> aaron.ballman wrote:
> > dsanders wrote:
> > > aaron.ballman wrote:
> > > > I don't think you should issue a second diagnostic on the same line. 
> > > > Instead, only issue the previous diagnostic with the fixit attached to 
> > > > it.
> > > I don't mind changing this but I thought I should mention that I'm 
> > > following the example set by the code generated by add_new_check.py which 
> > > has the diagnostic separate from the note with the fixit:
> > > ```
> > >   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently 
> > > awesome")
> > >   << MatchedDecl;
> > >   diag(MatchedDecl->getLocation(), "insert 'awesome'", 
> > > DiagnosticIDs::Note)
> > >   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), 
> > > "awesome_");
> > > ```
> > > Is the example doing it the right way?
> > That script is intended to generate boilerplate so that you don't have to 
> > and to show a very minimal example of how to print output. So it's both 
> > correct and not particularly helpful for real checks at the same time, if 
> > that makes sense.
> I guess it makes sense to have one example of a warning and one of a note. It 
> might be worth adding a comment suggesting to those new to clang-tidy that 
> they should try to emit a single 'this is wrong; do this' diagnostic rather 
> than the two separate ones from the example but that's not for this patch at 
> least.
> 
> Thanks
Yeah, I wouldn't be opposed to a patch to the python script to make that more 
clear. Or dropping the note entirely under the assumption the user can look at 
the `diag()` signature to see how to make warnings vs errors vs notes.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:21
+  auto RegisterClassMatch = hasType(
+  cxxRecordDecl(hasName("llvm::Register")).bind("registerClassDecl"));
+

Should be `::llvm::Register` but not hugely important as I doubt we care too 
terribly much about inner namespaces named `llvm`.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:31
+  has(memberExpr(hasDeclaration(
+  cxxConversionDecl().bind("operatorDecl"
+  .bind("var"))),

You don't seem to be using the `operatorDecl` binding -- you can remove it, if 
it's not intended to be used.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:45
+if (const auto *Namespace = dyn_cast(Context))
+  if (Namespace->getQualifiedNameAsString() == "llvm")
+Replacement = "Register";

This is a fairly expensive operation compared to calling `getName()`; are there 
situations where you think you need the extra work to be done? (Same comment 
below.)



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst:10
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+

initialize -> initialization



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm

I'd also like to see some tests like:
```
void func(unsigned Reg);

struct S {
  unsigned Reg;

  S(unsigned Reg) : Reg(Reg) {}
};

void other_func() {
  func(getReg());
  S s{getReg()};
  s.Reg = getReg();
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders marked 3 inline comments as done.
dsanders added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:48-51
+for (const auto *UsingDirective: Context->using_directives())
+  if (UsingDirective->getNominatedNamespace()
+  ->getQualifiedNameAsString() == "llvm")
+Replacement = "Register";

dsanders wrote:
> There's something not quite right here but I haven't figured out what yet. 
> One of the tests (apply_2()) is failing (which is odd because I ran `ninja 
> check-clang-tools` before posting the patch) because the `using namespace 
> llvm` in the function scope doesn't show up in the DeclContext for the 
> function. Am I looking in the wrong place for it?
> 
> More generally, I suspect there's an easier way to omit the `llvm::` than the 
> way I'm using on lines 43-52.
I've given in on trying to elide the `llvm::` for this case as the `using 
namespace llvm` inside a function doesn't seem to be easily discoverable and 
doesn't occur in practice



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+  << Replacement

aaron.ballman wrote:
> dsanders wrote:
> > aaron.ballman wrote:
> > > I don't think you should issue a second diagnostic on the same line. 
> > > Instead, only issue the previous diagnostic with the fixit attached to it.
> > I don't mind changing this but I thought I should mention that I'm 
> > following the example set by the code generated by add_new_check.py which 
> > has the diagnostic separate from the note with the fixit:
> > ```
> >   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
> >   << MatchedDecl;
> >   diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
> >   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
> > ```
> > Is the example doing it the right way?
> That script is intended to generate boilerplate so that you don't have to and 
> to show a very minimal example of how to print output. So it's both correct 
> and not particularly helpful for real checks at the same time, if that makes 
> sense.
I guess it makes sense to have one example of a warning and one of a note. It 
might be worth adding a comment suggesting to those new to clang-tidy that they 
should try to emit a single 'this is wrong; do this' diagnostic rather than the 
two separate ones from the example but that's not for this patch at least.

Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 214476.
dsanders added a comment.

- Give in on eliding the llvm:: for `using namespace llvm` inside a function 
They don't appear in the DeclContext and I've been unable to find a way to 
reasonably detect them Fortunately they don't occur in practice


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+using namespace llvm;
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,60 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // FIXME: Function-scoped UsingDirectiveDecl's don't appear to be in the
+  //DeclContext for the function so we can't elide the llvm:: in this
+  //case. Fortunately, it doesn't actually occur in the LLVM code base.
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: llvm::Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg1 = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void done_2() {
+  using namespace llvm;
+  Register Reg2 = getReg();
+  // CHECK-FIXES: done_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg2 = getReg();
+}
+
+namespace llvm {
+void done_3() {
+  Register Reg3 = getReg();
+  // CHECK-FIXES: done_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - llvm-prefer-register-over-unsigned
+
+llvm-prefer-register-over-unsigned
+==
+
+Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
+them to use ``Register``.
+
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+
+.. code-block:: c++
+
+  void example(MachineOperand ) {
+unsigned Reg = MO.getReg();
+...
+  }
+
+becomes:
+
+.. code-block:: c++
+
+  void example(MachineOperand ) {
+Register Reg = MO.getReg();
+...
+  }
+

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+  << Replacement

dsanders wrote:
> aaron.ballman wrote:
> > I don't think you should issue a second diagnostic on the same line. 
> > Instead, only issue the previous diagnostic with the fixit attached to it.
> I don't mind changing this but I thought I should mention that I'm following 
> the example set by the code generated by add_new_check.py which has the 
> diagnostic separate from the note with the fixit:
> ```
>   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
>   << MatchedDecl;
>   diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
>   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
> ```
> Is the example doing it the right way?
That script is intended to generate boilerplate so that you don't have to and 
to show a very minimal example of how to print output. So it's both correct and 
not particularly helpful for real checks at the same time, if that makes sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders marked 4 inline comments as done.
dsanders added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:48-51
+for (const auto *UsingDirective: Context->using_directives())
+  if (UsingDirective->getNominatedNamespace()
+  ->getQualifiedNameAsString() == "llvm")
+Replacement = "Register";

There's something not quite right here but I haven't figured out what yet. One 
of the tests (apply_2()) is failing (which is odd because I ran `ninja 
check-clang-tools` before posting the patch) because the `using namespace llvm` 
in the function scope doesn't show up in the DeclContext for the function. Am I 
looking in the wrong place for it?

More generally, I suspect there's an easier way to omit the `llvm::` than the 
way I'm using on lines 43-52.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 214276.
dsanders added a comment.

Changed diagnostic message and merged the fixit into the original diagnostic
Made the variable names more distinct in the tests
Added a test to cover `using namespace llvm` in the global namespace
Slight correction to the iteration over the contexts to avoid skipping one
clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+using namespace llvm;
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg1 = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void done_2() {
+  using namespace llvm;
+  Register Reg2 = getReg();
+  // CHECK-FIXES: done_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg2 = getReg();
+}
+
+namespace llvm {
+void done_3() {
+  Register Reg3 = getReg();
+  // CHECK-FIXES: done_3()
+  // CHECK-FIXES-NEXT: Register Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - llvm-prefer-register-over-unsigned
+
+llvm-prefer-register-over-unsigned
+==
+
+Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
+them to use ``Register``.
+
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+
+.. code-block:: c++
+
+  void example(MachineOperand ) {
+unsigned Reg = MO.getReg();
+...
+  }
+
+becomes:
+
+.. code-block:: c++
+
+  void example(MachineOperand ) {
+Register Reg = MO.getReg();
+...
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders marked an inline comment as done.
dsanders added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+  << Replacement

aaron.ballman wrote:
> I don't think you should issue a second diagnostic on the same line. Instead, 
> only issue the previous diagnostic with the fixit attached to it.
I don't mind changing this but I thought I should mention that I'm following 
the example set by the code generated by add_new_check.py which has the 
diagnostic separate from the note with the fixit:
```
  diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
  << MatchedDecl;
  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
  << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
```
Is the example doing it the right way?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:40
+
+  diag(UserVarDecl->getLocation(), "var %0 is %1 but holds a register")
+  << UserVarDecl << *VarType;

How about `variable %0 declared as %1; use '%2' instead` and move it below to 
where the fix-it is issued?



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:44
+  const DeclContext *Context = UserVarDecl->getDeclContext();
+  while((Context = Context->getParent()) != nullptr) {
+if (const auto *Namespace = dyn_cast(Context))

Formatting looks off here, you should run the patch through clang-format.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+  << Replacement

I don't think you should issue a second diagnostic on the same line. Instead, 
only issue the previous diagnostic with the fixit attached to it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 214194.
dsanders marked 2 inline comments as done.
dsanders added a comment.

UsingDirectiveDecl -> auto
` -> ``
Split LLVM changes into another patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: var 'Reg' is 'unsigned int' but holds a register [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: var 'Reg' is 'unsigned int' but holds a register [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: var 'Reg' is 'unsigned int' but holds a register [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
+
+void done_2() {
+  using namespace llvm;
+  Register Reg = getReg();
+  // CHECK-FIXES: done_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+namespace llvm {
+void done_3() {
+  Register Reg = getReg();
+  // CHECK-FIXES: done_3()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - llvm-prefer-register-over-unsigned
+
+llvm-prefer-register-over-unsigned
+==
+
+Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
+them to use ``Register``.
+
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+
+.. code-block:: c++
+
+  void example(MachineOperand ) {
+unsigned Reg = MO.getReg();
+...
+  }
+
+becomes:
+
+.. code-block:: c++
+
+  void example(MachineOperand ) {
+Register Reg = MO.getReg();
+...
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -274,6 +274,7 @@
llvm-include-order
llvm-namespace-comment
llvm-prefer-isa-or-dyn-cast-in-conditionals
+   llvm-prefer-register-over-unsigned
llvm-twine-local
misc-definitions-in-headers
misc-misplaced-const
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -79,6 +79,12 @@
   Finds uses of deprecated Googletest APIs with names containing ``case`` and
   replaces them with equivalent APIs with ``suite``.
 
+- New :doc:`llvm-prefer-register-over-unsigned
+  ` check.
+
+  Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
+  them to use ``Register``
+
 
 Improvements to include-fixer
 -
Index: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
@@ -0,0 +1,36 @@
+//===--- PreferRegisterOverUnsignedCheck.h - clang-tidy -*- C++ -*-===//
+//
+// 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I think patch should be split at least on Clang-tidy check and results of its 
run on LLVM code. Probably per-target patches is even better solution.




Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:48
+Replacement = "Register";
+for (const UsingDirectiveDecl *UsingDirective: Context->using_directives())
+  if (UsingDirective->getNominatedNamespace()

You could use const auto * for iterators.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:85
+
+  Finds historical use of `unsigned` to hold vregs and physregs and rewrites
+  them to use `Register`

Please use double back-ticks to highlight language constructs. Same in 
documentation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders added a comment.

In D65919#1621477 , @Eugene.Zelenko 
wrote:

> I think patch should be split at least on Clang-tidy check and results of its 
> run on LLVM code. Probably per-target patches is even better solution.


I've split the LLVM changes out into D65962 . 
I expect it will be split up further for commit purposes if nothing else but 
I've kept it in a single review for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65919/new/

https://reviews.llvm.org/D65919



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits