[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@njames93 Is there anything else I should address?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132461

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

[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@njames93 Friendly ping. The patch has addressed all comments and remained 
unchanged for 2 weeks. I would like to land it latest next week. If you are 
happy with the patch, please accept the review. Alternatively, please let me 
know if you intend to continue reviewing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132461

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

[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 460419.
carlosgalvezp added a comment.

Adjust warning message for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132461

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy -check-suffixes=DEFAULT   %s cppcoreguidelines-avoid-do-while %t
+// RUN: %check_clang_tidy -check-suffixes=IGNORE-MACROS %s cppcoreguidelines-avoid-do-while %t -- -config='{CheckOptions: [{key: cppcoreguidelines-avoid-do-while.IgnoreMacros, value: true}]}'
+
+#define FOO(x) \
+  do { \
+  } while (x != 0)
+
+#define BAR_0(x) \
+  do { \
+bar(x); \
+  } while (0)
+
+#define BAR_FALSE(x) \
+  do { \
+bar(x); \
+  } while (false)
+
+void bar(int);
+int baz();
+
+void foo()
+{
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
+do {
+
+} while(0);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(1);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(-1);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(false);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(true);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+int x1{0};
+do {
+  x1 = baz();
+} while (x1 > 0);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while (x1 != 0);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+constexpr int x2{0};
+do {
+
+} while (x2);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+constexpr bool x3{false};
+do {
+
+} while (x3);
+
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+FOO(x1);
+
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+BAR_0(x1);
+
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+BAR_FALSE(x1);
+}
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
@@ -177,6 +177,7 @@
`concurrency-mt-unsafe `_,
`concurrency-thread-canceltype-asynchronous `_,
`cppcoreguidelines-avoid-const-or-ref-data-members `_,
+   `cppcoreguidelines-avoid-do-while `_,
`cppcoreguidelines-avoid-goto `_,
`cppcoreguidelines-avoid-non-const-global-variables `_,
`cppcoreguidelines-init-variables `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst
@@ -0,0 +1,38 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-do-while
+
+cppcoreguidelines-avoid-do-while
+
+
+Warns when using ``do-while`` loops. They are less readable than plain ``while``
+loops, since the termination condition is at the end and the condition is not
+checked prior to the first iteration. Thi

[PATCH] D30538: Add documentation for -fno-strict-aliasing

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.
Herald added a subscriber: jeroen.dobbelaere.
Herald added a project: All.

Hi, is this commit still valid? Why hasn't it been pushed?


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

https://reviews.llvm.org/D30538

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 442015.
carlosgalvezp marked 2 inline comments as done.
carlosgalvezp added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -0,0 +1,152 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-const-or-ref-data-members %t
+namespace std {
+template 
+struct reference_wrapper {};
+} // namespace std
+
+namespace gsl {
+template 
+struct not_null {};
+} // namespace gsl
+
+struct Ok {
+  int i;
+  int *p;
+  const int *pc;
+  std::reference_wrapper r;
+  gsl::not_null n;
+};
+
+struct ConstMember {
+  const int c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
+};
+
+struct LvalueRefMember {
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+};
+
+struct ConstRefMember {
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+};
+
+struct RvalueRefMember {
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstAndRefMembers {
+  int const c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct Foo {};
+
+struct Ok2 {
+  Foo i;
+  Foo *p;
+  const Foo *pc;
+  std::reference_wrapper r;
+  gsl::not_null n;
+};
+
+struct ConstMember2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+};
+
+struct LvalueRefMember2 {
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+};
+
+struct ConstRefMember2 {
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+};
+
+struct RvalueRefMember2 {
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstAndRefMembers2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+using ConstType = const int;
+using RefType = int &;
+using ConstRefType = const int &;
+using RefRefType = int &&;
+
+struct WithAlias {
+  ConstType c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  RefType lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: member 'lr' is a reference
+  ConstRefType cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: member 'cr' is a reference
+  RefRefType rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template 
+using Array = int[N];
+
+struct ConstArrayMember {
+  const Array<1> c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: member 'c' is const qualified
+};
+
+struct LvalueRefArrayMember {
+  Array<2> &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'lr' is a reference
+};
+
+struct ConstLvalueRefArrayMember {
+  Array<3> const &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: member 'cr' is a reference
+};
+
+struct RvalueRefArrayMember {
+  Array<4> &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template 
+struct TemplatedOk {
+  T t;
+};
+
+template 
+struct TemplatedConst {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is const qualified
+};
+
+template 
+struct TemplatedRef {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is a reference
+};
+
+TemplatedOk t1{};
+TemplatedConst t2{123};
+TemplatedRef t3{123};
+TemplatedRef t4{123};
+TemplatedRef t5{t1.t};
Index: 

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@LegalizeAdulthood Thanks for the review! I've now rebased and addressed your 
comments. I've also verify the docs with the command you suggested, I was 
missing `-DLLVM_ENABLE_SPHINX` in the cmake command.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.
Herald added a reviewer: NoQ.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst:10
+`CppCoreGuidelines ES.25 
`_
+and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) 
`_.
+

AUTOSAR mentions should be removed as per previous comment from @aaron.ballman 


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

https://reviews.llvm.org/D54943

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 448323.
carlosgalvezp added a comment.

Rebase onto latest main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -0,0 +1,157 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-const-or-ref-data-members %t
+namespace std {
+template 
+struct unique_ptr {};
+
+template 
+struct shared_ptr {};
+} // namespace std
+
+namespace gsl {
+template 
+struct not_null {};
+} // namespace gsl
+
+struct Ok {
+  int i;
+  int *p;
+  const int *pc;
+  std::unique_ptr up;
+  std::shared_ptr sp;
+  gsl::not_null n;
+};
+
+struct ConstMember {
+  const int c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
+};
+
+struct LvalueRefMember {
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+};
+
+struct ConstRefMember {
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+};
+
+struct RvalueRefMember {
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstAndRefMembers {
+  int const c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct Foo {};
+
+struct Ok2 {
+  Foo i;
+  Foo *p;
+  const Foo *pc;
+  std::unique_ptr up;
+  std::shared_ptr sp;
+  gsl::not_null n;
+};
+
+struct ConstMember2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+};
+
+struct LvalueRefMember2 {
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+};
+
+struct ConstRefMember2 {
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+};
+
+struct RvalueRefMember2 {
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstAndRefMembers2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+using ConstType = const int;
+using RefType = int &;
+using ConstRefType = const int &;
+using RefRefType = int &&;
+
+struct WithAlias {
+  ConstType c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  RefType lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: member 'lr' is a reference
+  ConstRefType cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: member 'cr' is a reference
+  RefRefType rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template 
+using Array = int[N];
+
+struct ConstArrayMember {
+  const Array<1> c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: member 'c' is const qualified
+};
+
+struct LvalueRefArrayMember {
+  Array<2> &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'lr' is a reference
+};
+
+struct ConstLvalueRefArrayMember {
+  Array<3> const &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: member 'cr' is a reference
+};
+
+struct RvalueRefArrayMember {
+  Array<4> &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template 
+struct TemplatedOk {
+  T t;
+};
+
+template 
+struct TemplatedConst {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is const qualified
+};
+
+template 
+struct TemplatedRef {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is a reference
+};
+
+TemplatedOk t1{};
+TemplatedConst t2{123};
+TemplatedRef t3{123};
+TemplatedRef t4{123};
+TemplatedRef t5

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@njames93 Would you mind reviewing? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Great feedback, thanks! I had some ideas on how to go around the issues, 
looking forward to your thoughts.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp:103-104
+struct WithAlias {
+  ConstType c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  RefType lr;

njames93 wrote:
> I feel like this could potentially confuse users. Maybe we should walk the 
> alias to show where the type was declared to be const/reference.
> Or, not a fan of this choice, don't diagnose these unless the user opts into 
> this behaviour.
Sounds reasonable! I see that Clang compiler does not print an "alias stack" in 
detail, for example here:

https://godbolt.org/z/8sqE4fM1v

For the sake of consistency and simplicity, would it make sense to print 
something similar here? Example:

  "warning: member 'c' of type 'ConstType' (aka 'const int') is const qualified"



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp:136-157
+template 
+struct TemplatedOk {
+  T t;
+};
+
+template 
+struct TemplatedConst {

njames93 wrote:
> This whole block displeases me. Warning on the instantiation isn't a great 
> idea and could confuse users.
> Would again need some expansion notes to explain why the warning is being 
> triggered, especially when if for 99% of the uses one of these structs has a 
> T which isn't const or a reference.
> Failing that just disabling the check in template instantiations would also 
> fix the issue.
I agree with the sentiment 100%. Unfortunately it's currently the only way to 
test this kind of code in clang-tidy AFAIK, see for example other existing 
checks:

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/google/readability-casting.cpp#L332

I fully agree on needing expansion notes, just like the Clang compiler does. A 
very similar question about it was recently posted:

https://discourse.llvm.org/t/clang-tidy-diagnostics-for-template-code/62909

In that sense I believe it would make sense to try and focus the efforts into 
implementing a good centralized solution instead of having it duplicated on 
different checks.

Until then, I believe it should be fairly easy to clarify the actual type as 
proposed in the above comment for aliases.

What do you think?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 448965.
carlosgalvezp added a comment.

- Display type information in the diagnostic.
- Rebase onto latest main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -0,0 +1,169 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-const-or-ref-data-members %t
+namespace std {
+template 
+struct unique_ptr {};
+
+template 
+struct shared_ptr {};
+} // namespace std
+
+namespace gsl {
+template 
+struct not_null {};
+} // namespace gsl
+
+struct Ok {
+  int i;
+  int *p;
+  const int *pc;
+  std::unique_ptr up;
+  std::shared_ptr sp;
+  gsl::not_null n;
+};
+
+struct ConstMember {
+  const int c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
+};
+
+struct LvalueRefMember {
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' of type 'int &' is a reference
+};
+
+struct ConstRefMember {
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' of type 'const int &' is a reference
+};
+
+struct RvalueRefMember {
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' of type 'int &&' is a reference
+};
+
+struct ConstAndRefMembers {
+  const int c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' of type 'const int' is const qualified
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' of type 'int &' is a reference
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' of type 'const int &' is a reference
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' of type 'int &&' is a reference
+};
+
+struct Foo {};
+
+struct Ok2 {
+  Foo i;
+  Foo *p;
+  const Foo *pc;
+  std::unique_ptr up;
+  std::shared_ptr sp;
+  gsl::not_null n;
+};
+
+struct ConstMember2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' of type 'const Foo' is const qualified
+};
+
+struct LvalueRefMember2 {
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' of type 'Foo &' is a reference
+};
+
+struct ConstRefMember2 {
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' of type 'const Foo &' is a reference
+};
+
+struct RvalueRefMember2 {
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' of type 'Foo &&' is a reference
+};
+
+struct ConstAndRefMembers2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' of type 'const Foo' is const qualified
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' of type 'Foo &' is a reference
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' of type 'const Foo &' is a reference
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' of type 'Foo &&' is a reference
+};
+
+using ConstType = const int;
+using RefType = int &;
+using ConstRefType = const int &;
+using RefRefType = int &&;
+
+struct WithAlias {
+  ConstType c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' of type 'ConstType' (aka 'const int') is const qualified
+  RefType lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: member 'lr' of type 'RefType' (aka 'int &') is a reference
+  ConstRefType cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: member 'cr' of type 'ConstRefType' (aka 'const int &') is a reference
+  RefRefType rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' of type 'RefRefType' (aka 'int &&') is a reference
+};
+
+template 
+using Array = int[N];
+
+struct ConstArrayMember {
+  const Array<1> c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: member 'c' of type 'const Array<1>' (aka 'const int[1]') is const qualified
+};
+
+struct LvalueRefArrayMember {
+  Array<2> &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'lr' of type 'Array<2> &' (aka 'int (&)[2]') is a reference
+};
+
+struct ConstLvalueRefArrayMember {
+  const Array<3> &cr;
+  // CHECK-MESSAGES: :[[@LIN

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@njames93  I've updated the patch with extra type information, let me know if 
you think it's good enough!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@njames93 Friendly ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Ok, thanks for the explanation! I'm mostly interested on the warning message, 
we've had situations before where the warning describes the problem **and** the 
solution, which can easily lead to confusion. From the tests I can see the 
message is quite generic "use an enum", so it won't push users to prefer one 
variant over the other.

I'd like to play a bit with the patch and see what pops in our codebase but 
somehow I get an error when downloading, do you happen to know what could be 
wrong? Alternatively if there's any other easy way to checkout the patch and 
test it :)

  $ arc patch D117522
   Exception 
  preg_match(): Passing null to parameter #2 ($subject) of type string is 
deprecated
  (Run with `--trace` for a full exception trace.)



> This check is more about implementing Enum.1 Prefer enumerations over macros.

Should a `cppcoreguidelines` alias be added in that case?


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

https://reviews.llvm.org/D117522

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks a lot @tonic for your update and for the work done in the last months ! 
It's indeed very sad to hear the news. It's a pity that the work will probably 
be duplicated in many local forks with sub-optimal solutions instead of a 
centralized, high-quality, peer-reviewed open-source solution.

I'm not very familiar with the terms so I'm not sure I fully understand the 
reasons why we are advised not to proceed. Could you clarify/elaborate on what 
it means "the exact patent burden is not disclosed"? I was hoping that the 
written consent we got from AUTOSAR would be a strong basis to move forward.


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

https://reviews.llvm.org/D112730

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D112730#3316646 , @carlosgalvezp 
wrote:

> Thanks a lot @tonic for your update and for the work done in the last months 
> ! It's indeed very sad to hear the news. It's a pity that the work will 
> probably be duplicated in many local forks with sub-optimal solutions instead 
> of a centralized, high-quality, peer-reviewed open-source solution.
>
> I'm not very familiar with the terms so I'm not sure I fully understand the 
> reasons why we are advised not to proceed. Could you clarify/elaborate on 
> what it means "the exact patent burden is not disclosed"? I was hoping that 
> the written consent we got from AUTOSAR would be a strong basis to move 
> forward.

In general it would also be good to know if there was any other impediment to 
move forward. This information would be extremely valuable to try and do better 
in the future - I'm thinking on the upcoming MISRA release.

Chris Tapp (chair of MISRA) has been involved in similar discussions 

 in the cfe-dev mailing list, so I think it would be beneficial for him to get 
this feedback and hopefully make the upcoming MISRA release easy to work with 
from this standpoint (if MISRA is interested in an open-source checker 
implementation, of course).

Looking forward to your thoughts :)


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

https://reviews.llvm.org/D112730

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@LegalizeAdulthood I've addressed your comments, is there anything that should 
be fixed before landing the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 444633.
carlosgalvezp added a comment.

- Rebase onto latest main.
- Remove references to std::reference_wrapper as alternative suggestions, as 
per: https://github.com/isocpp/CppCoreGuidelines/issues/1919


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -0,0 +1,157 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-const-or-ref-data-members %t
+namespace std {
+template 
+struct unique_ptr {};
+
+template 
+struct shared_ptr {};
+} // namespace std
+
+namespace gsl {
+template 
+struct not_null {};
+} // namespace gsl
+
+struct Ok {
+  int i;
+  int *p;
+  const int *pc;
+  std::unique_ptr up;
+  std::shared_ptr sp;
+  gsl::not_null n;
+};
+
+struct ConstMember {
+  const int c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
+};
+
+struct LvalueRefMember {
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+};
+
+struct ConstRefMember {
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+};
+
+struct RvalueRefMember {
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstAndRefMembers {
+  int const c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  int &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+  const int &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct Foo {};
+
+struct Ok2 {
+  Foo i;
+  Foo *p;
+  const Foo *pc;
+  std::unique_ptr up;
+  std::shared_ptr sp;
+  gsl::not_null n;
+};
+
+struct ConstMember2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+};
+
+struct LvalueRefMember2 {
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+};
+
+struct ConstRefMember2 {
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+};
+
+struct RvalueRefMember2 {
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstAndRefMembers2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  Foo &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'lr' is a reference
+  const Foo &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'cr' is a reference
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+using ConstType = const int;
+using RefType = int &;
+using ConstRefType = const int &;
+using RefRefType = int &&;
+
+struct WithAlias {
+  ConstType c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  RefType lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: member 'lr' is a reference
+  ConstRefType cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: member 'cr' is a reference
+  RefRefType rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template 
+using Array = int[N];
+
+struct ConstArrayMember {
+  const Array<1> c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: member 'c' is const qualified
+};
+
+struct LvalueRefArrayMember {
+  Array<2> &lr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'lr' is a reference
+};
+
+struct ConstLvalueRefArrayMember {
+  Array<3> const &cr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: member 'cr' is a reference
+};
+
+struct RvalueRefArrayMember {
+  Array<4> &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template 
+struct TemplatedOk {
+  T t;
+};
+
+template 
+struct TemplatedConst {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is const qualified
+};
+
+template 
+struct TemplatedRef {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warnin

[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Ping to reviewers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.
Herald added a subscriber: jeroen.dobbelaere.

Hi!

I've been following this issue about aliases in clang-tidy through different 
bug reports and commit attempts. I see that the author abandoned this commit 
without an explanation. What's the reason? Where can I follow the continuation 
of this discussion, is there a email list or something?

I believe making sure that clang-tidy only runs once instance of the checks is 
important, it can dramatically reduce the runtime. This can have a significant 
impact in CI working with large codebases. It also doesn't make sense that 
alias checks aren't identical and don't detect the same things. As it has been 
mentioned above, users should not carry the burden of knowing and maintaining 
what is aliasing what at any point in time, that's an internal implementation 
detail of clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566

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

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Fully agree. Such an opt-in feature shouldn't break things. I'd be happy to 
implement it if people think it's a good idea! Would need some hand-holding 
though, never touched the LLVM codebase before :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72566

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

[PATCH] D110493: [clang-tidy] Fix bug 51790 in readability-uppercase-literal-suffix

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
carlosgalvezp added reviewers: llvm-commits, cfe-commits, steveire, lebedev.ri, 
alexfh, alexfh_.
carlosgalvezp added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
carlosgalvezp requested review of this revision.

A bisect determined that the bug was introduced here:
https://github.com/llvm/llvm-project/commit/ea2225a10be986d226e041d20d36dff17e78daed

Unfortunately that patch can no longer be reverted on top of the main branch, 
so add a fix instead. Add a unit test to avoid regression in the future.

  

Fixes https://bugs.llvm.org/show_bug.cgi?id=51790


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110493

Files:
  clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
@@ -270,3 +270,17 @@
 void d();
 void d() { c(); }
 } // namespace
+
+// Check that non-type template parameters do not cause any diags.
+// https://bugs.llvm.org/show_bug.cgi?id=51790
+template 
+struct Vector {
+  static constexpr int kCapacity = capacity;
+};
+
+template 
+constexpr int Vector::kCapacity;
+
+void test_vector() {
+  int x = Vector<10>::kCapacity;
+}
Index: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
@@ -134,6 +134,11 @@
   CharSourceRange::getTokenRange(*Range), SM, LO, &Invalid);
   assert(!Invalid && "Failed to retrieve the source text.");
 
+  // Make sure the first character is actually a digit
+  // https://bugs.llvm.org/show_bug.cgi?id=51790
+  if (!std::isdigit(static_cast(LiteralSourceText.front(
+return llvm::None;
+
   size_t Skip = 0;
 
   // Do we need to ignore something before actually looking for the suffix?


Index: clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
@@ -270,3 +270,17 @@
 void d();
 void d() { c(); }
 } // namespace
+
+// Check that non-type template parameters do not cause any diags.
+// https://bugs.llvm.org/show_bug.cgi?id=51790
+template 
+struct Vector {
+  static constexpr int kCapacity = capacity;
+};
+
+template 
+constexpr int Vector::kCapacity;
+
+void test_vector() {
+  int x = Vector<10>::kCapacity;
+}
Index: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
@@ -134,6 +134,11 @@
   CharSourceRange::getTokenRange(*Range), SM, LO, &Invalid);
   assert(!Invalid && "Failed to retrieve the source text.");
 
+  // Make sure the first character is actually a digit
+  // https://bugs.llvm.org/show_bug.cgi?id=51790
+  if (!std::isdigit(static_cast(LiteralSourceText.front(
+return llvm::None;
+
   size_t Skip = 0;
 
   // Do we need to ignore something before actually looking for the suffix?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110493: [clang-tidy] Fix bug 51790 in readability-uppercase-literal-suffix

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I personally think this fix should happen when doing the match, not when 
analyzing the matches. This is my first patch to LLVM and I'm not knowledgeable 
in AST so I don't really know how to go about that :) Please come with 
suggestions if there's a better way to do this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110493

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

[PATCH] D110493: [clang-tidy] Fix bug 51790 in readability-uppercase-literal-suffix

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 375242.
carlosgalvezp added a comment.

Fixed review comments


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

https://reviews.llvm.org/D110493

Files:
  clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
@@ -270,3 +270,29 @@
 void d();
 void d() { c(); }
 } // namespace
+
+// Check that non-type template parameters do not cause any diags.
+// https://bugs.llvm.org/show_bug.cgi?id=51790
+template 
+struct Vector {
+  static constexpr int kCapacity = capacity;
+};
+
+template 
+constexpr int Vector::kCapacity;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:22: warning: integer literal has suffix 
'ity', which is not uppercase
+
+template 
+struct Foo {
+  static constexpr int kFoo = foo1u;
+};
+
+template 
+constexpr int Foo::kFoo;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:19: warning: integer literal has suffix 
'u', which is not uppercase
+
+// The template needs to be instantiated for diagnostics to show up
+void test_non_type_template_parameter() {
+  int x = Vector<10>::kCapacity;
+  int f = Foo<10>::kFoo;
+}
Index: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
@@ -134,6 +134,11 @@
   CharSourceRange::getTokenRange(*Range), SM, LO, &Invalid);
   assert(!Invalid && "Failed to retrieve the source text.");
 
+  // Make sure the first character is actually a digit, instead of
+  // something else, like a non-type template parameter.
+  if (!std::isdigit(static_cast(LiteralSourceText.front(
+return llvm::None;
+
   size_t Skip = 0;
 
   // Do we need to ignore something before actually looking for the suffix?


Index: clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-uppercase-literal-suffix-integer.cpp
@@ -270,3 +270,29 @@
 void d();
 void d() { c(); }
 } // namespace
+
+// Check that non-type template parameters do not cause any diags.
+// https://bugs.llvm.org/show_bug.cgi?id=51790
+template 
+struct Vector {
+  static constexpr int kCapacity = capacity;
+};
+
+template 
+constexpr int Vector::kCapacity;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:22: warning: integer literal has suffix 'ity', which is not uppercase
+
+template 
+struct Foo {
+  static constexpr int kFoo = foo1u;
+};
+
+template 
+constexpr int Foo::kFoo;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:19: warning: integer literal has suffix 'u', which is not uppercase
+
+// The template needs to be instantiated for diagnostics to show up
+void test_non_type_template_parameter() {
+  int x = Vector<10>::kCapacity;
+  int f = Foo<10>::kFoo;
+}
Index: clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
@@ -134,6 +134,11 @@
   CharSourceRange::getTokenRange(*Range), SM, LO, &Invalid);
   assert(!Invalid && "Failed to retrieve the source text.");
 
+  // Make sure the first character is actually a digit, instead of
+  // something else, like a non-type template parameter.
+  if (!std::isdigit(static_cast(LiteralSourceText.front(
+return llvm::None;
+
   size_t Skip = 0;
 
   // Do we need to ignore something before actually looking for the suffix?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110493: [clang-tidy] Fix bug in readability-uppercase-literal-suffix

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Awesome, thanks a lot for the review! :) Sure, you can use the following to 
commit on my behalf, since I don't think I have such permissions:

Carlos Galvez
carlosgalv...@gmail.com


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

https://reviews.llvm.org/D110493

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

[PATCH] D110493: [clang-tidy] Fix bug in readability-uppercase-literal-suffix

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Super, thanks! It went really smooth :)

Newbye question: I noticed that `[clang-tidy]` is no longer part of the commit 
message, I thought the intention was to keep it to more easily glance through 
the commit history?


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

https://reviews.llvm.org/D110493

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

[PATCH] D110493: [clang-tidy] Fix bug in readability-uppercase-literal-suffix

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the clarification, makes sense! I got the notification about the 
build failure, thanks for fixing. I actually added it in the first place but 
then noticed that checkers typically didn't include any STL header so I thought 
maybe there was a restriction on their usage or something. The code compiled 
just fine when running the "clang-check-tools" target. Good to know for the 
next time :)

PS: I've also reported the build errors in pre-merge, maybe they would have 
caught it earlier.


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

https://reviews.llvm.org/D110493

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

[PATCH] D110614: [clang-tidy] Fix false positive in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
carlosgalvezp added reviewers: mgartmann, aaron.ballman, alexfh, alexfh_.
carlosgalvezp added a project: clang-tools-extra.
Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai.
carlosgalvezp requested review of this revision.

Incorrectly triggers for template classes that inherit from a base class that 
has virtual destructor.

  

Any class inheriting from a base that has a virtual destructor will have their 
destructor also virtual, as per the Standard, so there's no need for them to 
explicitly declare it as virtual.

  

https://timsong-cpp.github.io/cppwp/n4140/class.dtor#9

  

> If a class has a base class with a virtual destructor, its destructor 
> (whether user- or implicitly-declared) is virtual.

  

Added unit test to prevent regression.

  

Fixes https://bugs.llvm.org/show_bug.cgi?id=51912


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,12 @@
   void m();
 };
 // inherits virtual method
+
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+void test_templates() {
+  TemplatedDerived x;
+}
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,15 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher BaseHasPublicVirtualDtor =
+  hasAnyBase(hasType(
+  cxxRecordDecl(has(cxxDestructorDecl(isPublic(), isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  BaseHasPublicVirtualDtor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,12 @@
   void m();
 };
 // inherits virtual method
+
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+void test_templates() {
+  TemplatedDerived x;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,15 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher BaseHasPublicVirtualDtor =
+  hasAnyBase(hasType(
+  cxxRecordDecl(has(cxxDestructorDecl(isPublic(), isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  BaseHasPublicVirtualDtor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 375545.
carlosgalvezp retitled this revision from "[clang-tidy] Fix false positive in 
cppcoreguidelines-virtual-class-destructor " to "[clang-tidy] Fix false 
positives in cppcoreguidelines-virtual-class-destructor ".
carlosgalvezp edited the summary of this revision.
carlosgalvezp added a comment.

Added additional test case for forward declarations, which is also broken on 
main.


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,23 @@
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug 
https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredClass' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredClass;
+
+struct ForwardDeclaredClass : PublicVirtualBaseStruct{
+
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived x;
+}
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,15 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher BaseHasPublicVirtualDtor =
+  hasAnyBase(hasType(
+  cxxRecordDecl(has(cxxDestructorDecl(isPublic(), isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  BaseHasPublicVirtualDtor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,23 @@
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredClass' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredClass;
+
+struct ForwardDeclaredClass : PublicVirtualBaseStruct{
+
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived x;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,15 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher BaseHasPublicVirtualDtor =
+  hasAnyBase(hasType(
+  cxxRecordDecl(has(cxxDestructorDecl(isPublic(), isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  BaseHasPublicVirtualDtor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 375546.
carlosgalvezp added a comment.

Rename Class to Struct


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,23 @@
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug 
https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct{
+
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived x;
+}
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,15 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher BaseHasPublicVirtualDtor =
+  hasAnyBase(hasType(
+  cxxRecordDecl(has(cxxDestructorDecl(isPublic(), isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  BaseHasPublicVirtualDtor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,23 @@
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct{
+
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived x;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,15 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher BaseHasPublicVirtualDtor =
+  hasAnyBase(hasType(
+  cxxRecordDecl(has(cxxDestructorDecl(isPublic(), isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  BaseHasPublicVirtualDtor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 375549.
carlosgalvezp added a comment.

-Remove "public" when checking for base virtual destructor, it's not relevant. 
-More consistent naming. 
-Fix formatting.


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,22 @@
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug 
https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived x;
+}
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,14 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher InheritsVirtualDestructor =
+  hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  InheritsVirtualDestructor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,22 @@
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived x;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,14 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher InheritsVirtualDestructor =
+  hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  InheritsVirtualDestructor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a subscriber: whisperity.
carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26
 
+  ast_matchers::internal::Matcher InheritsVirtualDestructor =
+  hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual());

aaron.ballman wrote:
> I'm confused as to why this is necessary -- this AST matcher calls through to 
> `CXXMethodDecl::isVirtual()` which is different from `isVirtualAsWritten()` 
> and should already account for inheriting the virtual specifier.
In the Bug report, @whisperity mentioned this problem could be somewhere else:

> Nevertheless, if you check the AST for the test code at 
> http://godbolt.org/z/6MqG4Kb85, we can see that the derived destructor is in 
> fact **not** marked virtual. So it's Sema which does not give this flag to 
> the AST-clients properly.

I don't even know what Sema is so I don't really know how to fix it at that 
level, and I wonder if it would break other things.

If anyone has some time to walk me through where the fix should go I'm happy to 
try it out!


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};

aaron.ballman wrote:
> I think there are more interesting template test cases that should be checked.
> ```
> // What to do when the derived type is definitely polymorphic
> // but the base class may or may not be?
> template 
> struct Derived : Ty {
>   virtual void func();
> };
> 
> struct S {};
> struct  T { virtual ~T(); };
> 
> void instantiate() {
>   Derived d1; // Diagnose
>   Derived d2; // Don't diagnose
> }
> ```
> 
Very interesting example.

The problem is that the diagnostic is shown where `Derived` is, not where the 
template is instantiated. How to go about that?

Seems like more testing and better diagnostics are needed to lower the amount 
of false positives, I wonder if we should disable the test meanwhile?


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};

aaron.ballman wrote:
> whisperity wrote:
> > carlosgalvezp wrote:
> > > aaron.ballman wrote:
> > > > I think there are more interesting template test cases that should be 
> > > > checked.
> > > > ```
> > > > // What to do when the derived type is definitely polymorphic
> > > > // but the base class may or may not be?
> > > > template 
> > > > struct Derived : Ty {
> > > >   virtual void func();
> > > > };
> > > > 
> > > > struct S {};
> > > > struct  T { virtual ~T(); };
> > > > 
> > > > void instantiate() {
> > > >   Derived d1; // Diagnose
> > > >   Derived d2; // Don't diagnose
> > > > }
> > > > ```
> > > > 
> > > Very interesting example.
> > > 
> > > The problem is that the diagnostic is shown where `Derived` is, not where 
> > > the template is instantiated. How to go about that?
> > > 
> > > Seems like more testing and better diagnostics are needed to lower the 
> > > amount of false positives, I wonder if we should disable the test 
> > > meanwhile?
> > First, let's try to see if printing, to the user, the matched record, 
> > prints `Derived` instead of just `Derived`, irrespective of the 
> > location. If the matcher matched the instantiation, the printing/formatting 
> > logic should **really** pick that up.
> > 
> > It would be very hard to get to the `VarDecl` with the specific type if 
> > your matcher logic starts the top-level matching from the type definitions 
> > (`cxxRecordDecl(...)`).
> > 
> > If we **really** want to put some sort of a diagnostic at the location at 
> > the places where the type is used, it could be done with another pass over 
> > the AST. However, that has an associated runtime cost, and also could 
> > create bazillions of `note: used here`-esque messages... But certainly 
> > doable. I believe this is `typeLoc`, but `typeLoc` is always a thing I 
> > never understand and every time I may end up using it it takes me a lot of 
> > reading to understand it for a short time to use it.
> > 
> > 
> > 
> > If the previous step failed, you could still go around in a much more 
> > convoluted way:
> > 
> > However, there is something called a `ClassTemplateSpecializationDecl` (see 
> > the AST for @aaron.ballman's example here: http://godbolt.org/z/9qd7d1rs6), 
> > which surely should have an associated matcher.
> > 
> > ```
> > | |-ClassTemplateSpecializationDecl  line:2:8 struct 
> > Derived definition
> > | | |-DefinitionData polymorphic literal has_constexpr_non_copy_move_ctor 
> > can_const_default_init
> > | | | |-DefaultConstructor exists non_trivial constexpr 
> > defaulted_is_constexpr
> > | | | |-CopyConstructor simple non_trivial has_const_param 
> > implicit_has_const_param
> > | | | |-MoveConstructor exists simple non_trivial
> > | | | |-CopyAssignment simple non_trivial has_const_param 
> > implicit_has_const_param
> > | | | |-MoveAssignment exists simple non_trivial
> > | | | `-Destructor simple irrelevant trivial
> > | | |-public 'S':'S'
> > | | |-TemplateArgument type 'S'
> > | | | `-RecordType 'S'
> > | | |   `-CXXRecord 'S'
> > | | |-CXXRecordDecl prev 0x55ebcec4e600  col:8 implicit 
> > struct Derived
> > ```
> > 
> > The location for the "template specialisation" is still the location of the 
> > primary template (as it is not an //explicit specialisation//), but at 
> > least somehow in the AST the information of //"Which type was this class 
> > template instantiated with?"// (`S`) is stored, so it is very likely that 
> > you can either match on this directly, or if you can't match that way, you 
> > could match all of these `ClassTemplateSpecializationDecl`s and check if 
> > their type parameter matches a `ProblematicClassOrStruct`.
> > 
> > Of course, this becomes extra nasty the moment we have multiple template 
> > parameters, or nested stuff, `template template`s (b...).
> > 
> > This will still not give you the location of the variable definition, but 
> > at least you would have in your hand the template 
> > specialisation/instantiation instance properly.
> Oh, I may have caused some confusion with the comments in my example, sorry 
> for that! I was imagining the diagnostics would be emitted on the line with 
> the class declaration, but I commented on which instantiation I thought 
> should diagnose. Being more explicit with what I was thinking:
> ```
> // What to do when the derived type is definitely polymorphic
> // but the base class may or may not be?
> template 
> struct Derived : Ty { // Diagnostic emitted here
>   virtual void func();
> };
> 
> struct S {};
> struct  T { virtual ~T(); };
> 
> void instantiate() {
>   Derived d1; // Instantiation causes a diagnostic above
>   Derived d2; // Instantiation does not cause a diagnostic above
> }
> ```
No worries, that was my unde

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp marked 5 inline comments as done.
carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:208
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;

whisperity wrote:
> What does this prefix do? (I've never seen this before! 😮)
I got the tip from @aaron.ballman :) Most likely it's not technically needed 
(the tests would catch unexpected warnings) but it expresses more clearly the 
expectations.


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 376116.
carlosgalvezp added a comment.

Added additional required tests.

I might take some more time to fix the matcher since I need to get acquainted 
with AST, clang-query, etc, etc


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct DerivedFromTemplateVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct 
InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public 
and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() = 
default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template 
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct 
InstantiationWithPublicNonVirtualBaseStruct;
+
+}  // namespace Bugzilla_51912
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,10 +23,14 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
+  ast_matchers::internal::Matcher InheritsVirtualDestructor =
+  hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual());
+
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
   unless(anyOf(
+  InheritsVirtualDestructor,
   has(cxxDestructorDecl(isPublic(), isVirtual())),
   has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
   .bind("ProblematicClassOrStruct"),


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived InstantiationWithInt;
+
+// Derived from template, bas

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

By the way, there is a compiler warning that is identical to this check. 
Wouldn't it make sense to just make both checks identical? Otherwise there's 
risk that they conflict one another:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L6726


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

So for some reason, in Sema, the following returns True:

`dtor->isVirtual() `

However the clang-tidy matcher :

`has(cxxDestructorDecl(isPublic(), isVirtual())),`

Doesn't match this, and that's why we get the FPs. Why would that be, isn't 
`isVirtual` doing the same as `dtor->isVirtual()`?


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Ahh, it seems that for templates, the class has this AST:

  |-ClassTemplateDecl  line:7:8 Derived
  | |-TemplateTypeParmDecl  col:20 typename depth 0 index 0 T
  | |-CXXRecordDecl  line:7:8 struct Derived definition
  | `-ClassTemplateSpecializationDecl  line:7:8 struct 
Derived definition

So the derived destructor only shows up in `ClassTemplateSpecializationDecl` - 
the `CXXRecordDecl` doesn't have it (therefore implicit public non-virtual) and 
that's why the check triggers.


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 376139.
carlosgalvezp added a comment.

Moved the condition of public-virtual / protected-non-virtual to the check 
stage instead of the match stage. This way is more similar to the equivalent 
Sema check and it passes all the tests. Let me know what you think!


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct DerivedFromTemplateVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct 
InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public 
and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() = 
default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template 
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct 
InstantiationWithPublicNonVirtualBaseStruct;
+
+} // namespace Bugzilla_51912
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -23,14 +23,10 @@
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
 
-  Finder->addMatcher(
-  cxxRecordDecl(
-  anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
-  unless(anyOf(
-  has(cxxDestructorDecl(isPublic(), isVirtual())),
-  has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
-  .bind("ProblematicClassOrStruct"),
-  this);
+  Finder->addMatcher(cxxRecordDecl(anyOf(has(cxxMethodDecl(isVirtual())),
+ InheritsVirtualMethod))
+ .bind("ProblematicClassOrStruct"),
+ this);
 }
 
 static CharSourceRange
@@ -152,6 +148,12 @@
   if (!Destructor)
 return;
 
+  if (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+   Destructor->isVirtual()) ||
+  ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+   !Destructor->isVirtual()))
+return;
+
   if (Destructor->getAccess() == AccessSpecifier::AS_private) {
 diag(MatchedClassOrStruct->getLocation(),
  "destructor of %0 is private and prevents using the type")


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 376168.
carlosgalvezp added a comment.

Created custom matcher and moved the logic from check to match stage. 
Documented why we can't simply match a CXXDestructorDecl.


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,47 @@
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 
'ForwardDeclaredStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'TemplatedDerived' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 
'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+template 
+struct DerivedFromTemplateVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct 
InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public 
and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() = 
default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template 
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct 
InstantiationWithPublicNonVirtualBaseStruct;
+
+} // namespace Bugzilla_51912
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -19,6 +19,21 @@
 namespace tidy {
 namespace cppcoreguidelines {
 
+AST_MATCHER(CXXRecordDecl, hasPublicVirtualOrProtectedNonVirtualDestructor) {
+  // We need to call Node.getDestructor() instead of matching a
+  // CXXDestructorDecl. Otherwise, tests will fail for class templates, since
+  // the primary template (not the specialization) always gets a non-virtual
+  // CXXDestructorDecl in the AST. https://bugs.llvm.org/show_bug.cgi?id=51912
+  const CXXDestructorDecl *Destructor = Node.getDestructor();
+  if (!Destructor)
+return false;
+
+  return (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+   Destructor->isVirtual()) ||
+  ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+   !Destructor->isVirtual()));
+}
+
 void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
   ast_matchers::internal::Matcher InheritsVirtualMethod =
   hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual());
@@ -26,9 +41,7 @@
   Finder->addMatcher(
   cxxRecordDecl(
   anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
-  unless(anyOf(
-  has(cxxDestructorDecl(isPublic(), isVirtual())),
-  has(cxxDestructorDecl(isProtected(), unless(isVirtual()))
+  unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
   .bind("ProblematicClassOrStruct"),
   this);
 }


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the feedback @whisperity , I'm learning so many new things :)


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp marked 3 inline comments as done.
carlosgalvezp added a comment.

Hi! Are there any more issues I should address? It would be nice to get it 
merged since it fixes quite a few FPs.

Tagging also @mgartmann as original author of this check in case he wants to 
comment.

Anyway this check will need to be extended in the future, since the C++ Core 
Guidelines has added a new bullet in their "Enforcement" section:
https://github.com/isocpp/CppCoreGuidelines/commit/e44a9fcbd40923e9d5d342e444cf8a811e4a3eae


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

https://reviews.llvm.org/D110614

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

[PATCH] D111041: Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
carlosgalvezp added reviewers: aaron.ballman, alexfh, lewmpk.
Herald added subscribers: shchenz, kbarton, nemanjai.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.

This requirement was introduced in the C++ Core guidelines in 2016:

https://github.com/isocpp/CppCoreGuidelines/commit/1894380d0abf4d7d49a983005647e0d41ecbf214

Then clang-tidy got updated to comply with the rule.

However in 2019 this decision was reverted:

https://github.com/isocpp/CppCoreGuidelines/commit/5fdfb20b760c5307bf86873798a146fcd7e912e6

Therefore we need to applying the correct configuration to
clang-tidy again.

This also makes this cppcoreguidelines check consistent
with the other 2 alias checks: hicpp-use-override and
modernize-use-override.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111041

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp


Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -106,9 +106,6 @@
 Opts["cppcoreguidelines-non-private-member-variables-in-classes."
  "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "true";
 
-Opts["cppcoreguidelines-explicit-virtual-functions."
- "IgnoreDestructors"] = "true";
-
 return Options;
   }
 };


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -106,9 +106,6 @@
 Opts["cppcoreguidelines-non-private-member-variables-in-classes."
  "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "true";
 
-Opts["cppcoreguidelines-explicit-virtual-functions."
- "IgnoreDestructors"] = "true";
-
 return Options;
   }
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111041: [clang-tidy] Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I wonder why I'm not getting automated pre-merge builds?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111041

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

[PATCH] D111041: [clang-tidy] Remove 'IgnoreDestructors = true' from cppcoreguidelines-explicit-virtual-functions

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the input!

> release note mentions

Sure thing!

> documentation changes

As we discussed in the separate mail thread, there is no documentation at all 
as to what default values an alias/main check has. Where should I put that? Or 
do you mean that now that all 3 checks (primary + 2 aliases) have identical 
defaults, I should document that in the main check documentation?

> test coverage

I was surprised to not find any unit test for this check, probably because it's 
already done by the unit test of the "main check"? Should add a new unit test, 
pretty much copy-pasting the test for the main check?

> evidence that this option is not being used by users

Not sure how to go about this one :) I guess we can't have a 100% guarantee 
that this won't break things for users, there will always be an edge case. 
Perhaps we should document it very clearly in the Release Notes as well as 
explaining how to revert to the old behavior if needed?

Since we are talking about a "guideline check" 
(cppcoreguidelines-explicit-virtual-functions) and a "good practice check" 
(modernize-use-override), I don't think it's unreasonable that users have 
enabled both checks. If so, the "modernize" check should already have this 
behavior as default and prompted users to make changes - the cppcoreguidelines 
was less strict, and now it would be as strict as the "modernize-use-override" 
check. In that scenario, this patch shouldn't break things for users. The 
scenario that would break things for users is when they only enable the 
cppcoreguidelines check alone.

The guidelines changed in 2019, I think after 2 years users wouldn't mind the 
tooling catching up :)

Since I'm new here and missed quite a few things, do you know if there's a 
checklist somewhere where I can read about what else I should include in the 
patch?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111041

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for your input @mgartmann ! No apologies needed, nothing is ever perfect 
on the first release :) Just wanted to check if you would agree with the fix or 
if there's some detail I've missed.

I'll add the tests asked by @whisperity


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

https://reviews.llvm.org/D110614

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

[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 377100.
carlosgalvezp added a comment.

Added test where an alias to the class template is created. @whisperity let me 
know if that's what you meant!


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

https://reviews.llvm.org/D110614

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -202,3 +202,73 @@
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct DerivedFromTemplateVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() = default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template 
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct InstantiationWithPublicNonVirtualBaseStruct;
+
+// Derived from template, base has virtual dtor, to be used in a typedef
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'DerivedFromTemplateVirtualBaseStruct2' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template 
+struct DerivedFromTemplateVirtualBaseStruct2 : T {
+  virtual void foo();
+};
+
+using DerivedFromTemplateVirtualBaseStruct2Typedef = DerivedFromTemplateVirtualBaseStruct2;
+DerivedFromTemplateVirtualBaseStruct2Typedef InstantiationWithPublicVirtualBaseStruct2;
+
+// Derived from template, base has *not* virtual dtor, to be used in a typedef
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct2' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct2' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct2 : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct2() = default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template 
+struct DerivedFromTemplateNonVirtualBaseStruct2 : T {
+  virtual void foo();
+};
+
+using DerivedFromTemplateNonVirtualBaseStruct2Typedef = DerivedFromTemplateNonVirtualBaseStruct2;
+DerivedFromTemplateNonVirtualBaseStruct2Typedef InstantiationWithPublicNonVirtualBaseStruct2;
+
+} // namespace Bugzilla_51912
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -19,6 +19,21 @@
 namespace tidy {
 namespace cppcoreguidelines {
 
+AST_MATCHER(CXXRecordDecl, hasPublicVirtualOrProtectedNonVirtualDestructor) {
+  // We need to call Node.getDestructor() instead of matching a
+  // CXXDestructorDecl. Otherwise, tests will fail for class templates, since
+  // 

[PATCH] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the review! I forgot to mention the Differential Revision in the 
commit message after pushing so this review stays open, is there any way I can 
add it now in some other way? I suppose we don't want force-push?


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

https://reviews.llvm.org/D112334

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

[PATCH] D112334: [clang-tidy] Suppress readability-static-access-through-instance for CUDA built-in variables

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@whisperity Understood, thanks for the help! I'll take more care to remember 
the reference next time :) I added it as a comment in Github for reference so 
there's at least some traceability.


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

https://reviews.llvm.org/D112334

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

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:328
 Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "false";
+Opts["cert-err33-c.CheckedFunctions"] = CertErr33CCheckedFunctions;
 return Options;

balazske wrote:
> carlosgalvezp wrote:
> > Same here
> This is correct: 'str34' before 'err33'.
No, `e` goes before `s`.  The existing checks are ordered: `dcl`, `oop`, `str`. 
So this check should go after `dcl16`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112409

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

[PATCH] D112409: [clang-tidy] Add check 'cert-err33-c'.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> I was not sure how the aliasing is to be handled if the check is not exactly 
> the same as the original.

I agree that the alias situation is a bit of a mess. I'll leave it to people 
with stronger opinion/experience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112409

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

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I've recently fiddled with the `GlobList` to enable globbing in `NOLINT` 
expressions, so I have it quite fresh and I think it could easily be re-used 
here. I'd be happy to implement this feature, should I (can I?) continue on 
this patch or create a brand new one?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654

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

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

On the other hand, switching from Regex to Glob will be quite a breaking change 
- everyone has `.*` in their config file, which will silently stop to work when 
switching to Glob and suddenly people won't get their headers linted. What do 
you think?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654

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

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Also, the new header filter expresion would need to be: `HeaderFilter: 
'*/mydir/*' instead of `HeaderFilter: 'mydir'`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654

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

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> everyone has .* in their config file, which will silently stop to work

Nevermind this. `.*` treated as a glob will turn into the regex `..*` which is 
equivalent.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654

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

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
carlosgalvezp added a project: clang-tools-extra.
Herald added subscribers: libc-commits, arphaman, zzheng, kbarton, xazax.hun, 
mgorny, nemanjai.
Herald added a project: libc-project.
carlosgalvezp requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.

Instead of regex, to allow for negative globs.

Rename HeaderFilterRegex to HeaderFilter,
to keep it consistent.

Fix existing usage of HeaderFilter in the codebase.

Add unit test excluding one directory.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112720

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-single-work-item-barrier.cpp
  
clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align-no-crash.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-unroll-loops.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.c
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-multi-fixes.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-transitive.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/clang-tidy/checkers/zircon-temporary-objects.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/.clang-tidy
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/1/.clang-tidy
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/3/.clang-tidy
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/overlapping/o.h
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-run-with-database.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/expand-modular-headers-ppcallbacks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-error-within-include.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/overlapping.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  libc/src/.clang-tidy
  libc/test/src/CMakeLists.txt

Index: libc/test/src/CMakeLists.txt
===
--- libc/test/src/CMakeLists.txt
+++ libc/test/src/CMakeLists.txt
@@ -115,7 +115,7 @@
 COMMAND $ --system-headers
   --checks=-*,llvmlibc-restrict-system-libc-headers
   "--extra-arg=-resource-dir=${COMPILER_RESOURCE_DIR}"
-  --header-filter=.*
+  --header-filter=*
   --warnings-as-errors=llvmlibc-*
   "-config={CheckOptions: [{key: llvmlibc-restrict-system-libc-headers.Includes, value: '-*, linux/*, asm/*.h, asm-generic/*.h'}]}"
   --quiet
Index: libc/src/.clang-tidy
===
--- libc/src/.clang-tidy
+++ libc/src/.clang-tidy
@@ -1,5 +1,5 @@
 Checks: '-*,llvmlibc-*'
-HeaderFilterRegex: '.*'
+HeaderFilter: '*'
 WarningsAsErrors: 'llvmlibc-*'
 CheckOptions:
   - key: llvmlibc-restrict-system-libc-headers.Includes
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -76,13 +76,13 @@
 TEST(ParseConfiguration, ValidConfiguration) {
   llvm::ErrorOr Options =
   parseConfiguration(llvm::MemoryBufferRef("Checks: \"-*,misc-*\"\n"
-   "HeaderFilterRegex: \".*\"\n"
+   "HeaderFilter: \"*\"\n"
 

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 383034.
carlosgalvezp added a comment.

Fixed typo.


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

https://reviews.llvm.org/D112720

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-single-work-item-barrier.cpp
  
clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align-no-crash.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-unroll-loops.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.c
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-multi-fixes.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-transitive.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/clang-tidy/checkers/zircon-temporary-objects.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/.clang-tidy
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/1/.clang-tidy
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/3/.clang-tidy
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/overlapping/o.h
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-run-with-database.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/expand-modular-headers-ppcallbacks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-error-within-include.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/overlapping.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  libc/src/.clang-tidy
  libc/test/src/CMakeLists.txt

Index: libc/test/src/CMakeLists.txt
===
--- libc/test/src/CMakeLists.txt
+++ libc/test/src/CMakeLists.txt
@@ -115,7 +115,7 @@
 COMMAND $ --system-headers
   --checks=-*,llvmlibc-restrict-system-libc-headers
   "--extra-arg=-resource-dir=${COMPILER_RESOURCE_DIR}"
-  --header-filter=.*
+  --header-filter=*
   --warnings-as-errors=llvmlibc-*
   "-config={CheckOptions: [{key: llvmlibc-restrict-system-libc-headers.Includes, value: '-*, linux/*, asm/*.h, asm-generic/*.h'}]}"
   --quiet
Index: libc/src/.clang-tidy
===
--- libc/src/.clang-tidy
+++ libc/src/.clang-tidy
@@ -1,5 +1,5 @@
 Checks: '-*,llvmlibc-*'
-HeaderFilterRegex: '.*'
+HeaderFilter: '*'
 WarningsAsErrors: 'llvmlibc-*'
 CheckOptions:
   - key: llvmlibc-restrict-system-libc-headers.Includes
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -76,13 +76,13 @@
 TEST(ParseConfiguration, ValidConfiguration) {
   llvm::ErrorOr Options =
   parseConfiguration(llvm::MemoryBufferRef("Checks: \"-*,misc-*\"\n"
-   "HeaderFilterRegex: \".*\"\n"
+   "HeaderFilter: \"*\"\n"
"AnalyzeTemporaryDtors: true\n"
"User: some.user",
"Options"));
   EXPECT_TRUE(!!Options);
   EXPECT_EQ("-*,misc-*", *Options->Checks);
-  EXPECT_EQ(".*", *Options->HeaderFilterRegex);
+  EXPECT_EQ("*", *Options->HeaderFilter);
   EXPECT_EQ("some.user", *Options-

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Here's a patch that uses Globs, let me know how you like it!
https://reviews.llvm.org/D112720


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654

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

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 383036.
carlosgalvezp added a comment.

Fixed formatting of release notes


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

https://reviews.llvm.org/D112720

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-internal-dependencies.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-no-namespace.cpp
  
clang-tools-extra/test/clang-tidy/checkers/altera-id-dependent-backward-branch.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-single-work-item-barrier.cpp
  
clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align-no-crash.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align.cpp
  clang-tools-extra/test/clang-tidy/checkers/altera-unroll-loops.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.c
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-header.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value-multi-fixes.cpp
  
clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-transitive.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
  clang-tools-extra/test/clang-tidy/checkers/zircon-temporary-objects.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/.clang-tidy
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/1/.clang-tidy
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/3/.clang-tidy
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/overlapping/o.h
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-run-with-database.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/expand-modular-headers-ppcallbacks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-error-within-include.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/overlapping.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  libc/src/.clang-tidy
  libc/test/src/CMakeLists.txt

Index: libc/test/src/CMakeLists.txt
===
--- libc/test/src/CMakeLists.txt
+++ libc/test/src/CMakeLists.txt
@@ -115,7 +115,7 @@
 COMMAND $ --system-headers
   --checks=-*,llvmlibc-restrict-system-libc-headers
   "--extra-arg=-resource-dir=${COMPILER_RESOURCE_DIR}"
-  --header-filter=.*
+  --header-filter=*
   --warnings-as-errors=llvmlibc-*
   "-config={CheckOptions: [{key: llvmlibc-restrict-system-libc-headers.Includes, value: '-*, linux/*, asm/*.h, asm-generic/*.h'}]}"
   --quiet
Index: libc/src/.clang-tidy
===
--- libc/src/.clang-tidy
+++ libc/src/.clang-tidy
@@ -1,5 +1,5 @@
 Checks: '-*,llvmlibc-*'
-HeaderFilterRegex: '.*'
+HeaderFilter: '*'
 WarningsAsErrors: 'llvmlibc-*'
 CheckOptions:
   - key: llvmlibc-restrict-system-libc-headers.Includes
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -76,13 +76,13 @@
 TEST(ParseConfiguration, ValidConfiguration) {
   llvm::ErrorOr Options =
   parseConfiguration(llvm::MemoryBufferRef("Checks: \"-*,misc-*\"\n"
-   "HeaderFilterRegex: \".*\"\n"
+   "HeaderFilter: \"*\"\n"
"AnalyzeTemporaryDtors: true\n"
"User: some.user",
"Options"));
   EXPECT_TRUE(!!Options);
   EXPECT_EQ("-*,misc-*", *Options->Checks);
-  EXPECT_EQ(".*", *Options->HeaderFilterRegex);
+  EXPECT_EQ("*", *Options->HeaderFilter);
   EXPECT_EQ(

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D112720#3093636 , @lebedev.ri 
wrote:

> Not in favor of general directions like this.
> Does it not break the existing uses (existing `.clang-tidy`'s e.g.)

Yep, it's a breaking change. The purpose of this patch is mostly to show how it 
would look like and see if that would be the desired result. There's 2 aspects 
of it:

- Rename of `HeaderFilterRegex` to `HeaderFilter`, to be consistent with 
`--header-filter`. If this is too much breakage, I'm OK with reverting.
- Replace regex with globs. Won't break the use of `.*`. It will break for the 
use case of having a parent `.clang-tidy` with `.*` and a child `.clang-tidy` 
with `my_folder/*\.h`. I personally always found the current interface 
confusing as a user. What regex should I write? Should I consider full paths, 
relative paths, does it matter?

I suppose I can keep the exact same behavior as master, but that will add 
additional tweaks and complexity to the `GlobList` class.


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

https://reviews.llvm.org/D112720

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

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Just to clarify, this patch is the solution proposed by @alexfh here: 
https://reviews.llvm.org/D34654#3022728


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

https://reviews.llvm.org/D112720

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
carlosgalvezp added reviewers: aaron.ballman, whisperity.
carlosgalvezp added a project: clang-tools-extra.
Herald added subscribers: armkevincheng, jsmolens, eric-k256, arphaman, 
rnkovacs, kbarton, xazax.hun, mgorny, nemanjai.
Herald added a reviewer: sjarus.
carlosgalvezp requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.

To allow checking the AUTOSAR C++14 Coding Guidelines.

Also an easy check, alias of the C++ Core Guidelines one.

Test and documentation included.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112730

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
  clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
  clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast,autosar-a5-2-4-reinterpret-cast %t
 
 int i = 0;
 void *j;
 void f() { j = reinterpret_cast(i); }
-// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [autosar-a5-2-4-reinterpret-cast,cppcoreguidelines-pro-type-reinterpret-cast]
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -60,6 +60,7 @@
 ``abseil-``Checks related to Abseil library.
 ``altera-``Checks related to OpenCL programming for FPGAs.
 ``android-``   Checks related to Android.
+``autosar-``   Checks related to AUTOSAR C++14 Coding Guidelines.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
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
@@ -35,6 +35,7 @@
`altera-single-work-item-barrier `_,
`altera-struct-pack-align `_, "Yes"
`altera-unroll-loops `_,
+   `autosar-a5-4-2-reinterpret-cast `_,
`android-cloexec-accept `_, "Yes"
`android-cloexec-accept4 `_,
`android-cloexec-creat `_, "Yes"
@@ -328,6 +329,7 @@
 .. csv-table:: Aliases..
:header: "Name", "Redirect", "Offers fixes"
 
+   `autosar-a5-4-2-reinterpret-cast `_, `cppcoreguidelines-pro-type-reinterpret-cast `_,
`cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_,
`cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_,
`cert-dcl03-c `_, `misc-static-assert `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - autosar-a5-4-2-reinterpret-cast
+.. meta::
+   :http-equiv=refresh: 5;URL=cppcoreguidelines-pro-type-reinterpret-cast.html
+
+autosar-a5-4-2-reinterpret-cast
+===
+
+The autosar-a5-4-2-reinterpret-cast check is an alias, please see
+`cppcoreguidelines-pro-type-reinterpret-cast `_
+for more information.
Index: clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
@@ -0,0 +1,27 @@
+set(LLVM_LINK_COMPONENTS
+  FrontendOpenMP
+  Support
+  )
+
+add_clang_library(clangTidyAutosarModule
+  AutosarTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUtils
+  clangTidyCppCoreGuidelinesModule
+
+  DEPENDS
+  omp_gen
+  )
+
+clang_target_link_libraries(clangTidyAutosarModule
+  PRIVATE
+  clangAnalysis
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTooling
+  clangStaticAnalyzerCheckers
+  )
Index: clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
==

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 383058.
carlosgalvezp added a comment.

Specify C++14 in the module description.


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

https://reviews.llvm.org/D112730

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
  clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
  clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast,autosar-a5-2-4-reinterpret-cast %t
 
 int i = 0;
 void *j;
 void f() { j = reinterpret_cast(i); }
-// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [autosar-a5-2-4-reinterpret-cast,cppcoreguidelines-pro-type-reinterpret-cast]
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -60,6 +60,7 @@
 ``abseil-``Checks related to Abseil library.
 ``altera-``Checks related to OpenCL programming for FPGAs.
 ``android-``   Checks related to Android.
+``autosar-``   Checks related to AUTOSAR C++14 Coding Guidelines.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
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
@@ -35,6 +35,7 @@
`altera-single-work-item-barrier `_,
`altera-struct-pack-align `_, "Yes"
`altera-unroll-loops `_,
+   `autosar-a5-4-2-reinterpret-cast `_,
`android-cloexec-accept `_, "Yes"
`android-cloexec-accept4 `_,
`android-cloexec-creat `_, "Yes"
@@ -328,6 +329,7 @@
 .. csv-table:: Aliases..
:header: "Name", "Redirect", "Offers fixes"
 
+   `autosar-a5-4-2-reinterpret-cast `_, `cppcoreguidelines-pro-type-reinterpret-cast `_,
`cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_,
`cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_,
`cert-dcl03-c `_, `misc-static-assert `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - autosar-a5-4-2-reinterpret-cast
+.. meta::
+   :http-equiv=refresh: 5;URL=cppcoreguidelines-pro-type-reinterpret-cast.html
+
+autosar-a5-4-2-reinterpret-cast
+===
+
+The autosar-a5-4-2-reinterpret-cast check is an alias, please see
+`cppcoreguidelines-pro-type-reinterpret-cast `_
+for more information.
Index: clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
@@ -0,0 +1,27 @@
+set(LLVM_LINK_COMPONENTS
+  FrontendOpenMP
+  Support
+  )
+
+add_clang_library(clangTidyAutosarModule
+  AutosarTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUtils
+  clangTidyCppCoreGuidelinesModule
+
+  DEPENDS
+  omp_gen
+  )
+
+clang_target_link_libraries(clangTidyAutosarModule
+  PRIVATE
+  clangAnalysis
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTooling
+  clangStaticAnalyzerCheckers
+  )
Index: clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
@@ -0,0 +1,38 @@
+//===--- AutosarTidyModule.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===-

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 383059.
carlosgalvezp added a comment.

Fix ordering in check list.


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

https://reviews.llvm.org/D112730

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
  clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
  clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast,autosar-a5-2-4-reinterpret-cast %t
 
 int i = 0;
 void *j;
 void f() { j = reinterpret_cast(i); }
-// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [autosar-a5-2-4-reinterpret-cast,cppcoreguidelines-pro-type-reinterpret-cast]
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -60,6 +60,7 @@
 ``abseil-``Checks related to Abseil library.
 ``altera-``Checks related to OpenCL programming for FPGAs.
 ``android-``   Checks related to Android.
+``autosar-``   Checks related to AUTOSAR C++14 Coding Guidelines.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
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
@@ -50,6 +50,7 @@
`android-cloexec-pipe2 `_,
`android-cloexec-socket `_,
`android-comparison-in-temp-failure-retry `_,
+   `autosar-a5-4-2-reinterpret-cast `_,
`boost-use-to-string `_, "Yes"
`bugprone-argument-comment `_, "Yes"
`bugprone-assert-side-effect `_,
@@ -328,6 +329,7 @@
 .. csv-table:: Aliases..
:header: "Name", "Redirect", "Offers fixes"
 
+   `autosar-a5-4-2-reinterpret-cast `_, `cppcoreguidelines-pro-type-reinterpret-cast `_,
`cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_,
`cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_,
`cert-dcl03-c `_, `misc-static-assert `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - autosar-a5-4-2-reinterpret-cast
+.. meta::
+   :http-equiv=refresh: 5;URL=cppcoreguidelines-pro-type-reinterpret-cast.html
+
+autosar-a5-4-2-reinterpret-cast
+===
+
+The autosar-a5-4-2-reinterpret-cast check is an alias, please see
+`cppcoreguidelines-pro-type-reinterpret-cast `_
+for more information.
Index: clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
@@ -0,0 +1,27 @@
+set(LLVM_LINK_COMPONENTS
+  FrontendOpenMP
+  Support
+  )
+
+add_clang_library(clangTidyAutosarModule
+  AutosarTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUtils
+  clangTidyCppCoreGuidelinesModule
+
+  DEPENDS
+  omp_gen
+  )
+
+clang_target_link_libraries(clangTidyAutosarModule
+  PRIVATE
+  clangAnalysis
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTooling
+  clangStaticAnalyzerCheckers
+  )
Index: clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
@@ -0,0 +1,38 @@
+//===--- AutosarTidyModule.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 383087.
carlosgalvezp added a comment.

Update release notes.


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

https://reviews.llvm.org/D112730

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
  clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast,autosar-a5-2-4-reinterpret-cast %t
 
 int i = 0;
 void *j;
 void f() { j = reinterpret_cast(i); }
-// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [autosar-a5-2-4-reinterpret-cast,cppcoreguidelines-pro-type-reinterpret-cast]
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -60,6 +60,7 @@
 ``abseil-``Checks related to Abseil library.
 ``altera-``Checks related to OpenCL programming for FPGAs.
 ``android-``   Checks related to Android.
+``autosar-``   Checks related to AUTOSAR C++14 Coding Guidelines.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
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
@@ -50,6 +50,7 @@
`android-cloexec-pipe2 `_,
`android-cloexec-socket `_,
`android-comparison-in-temp-failure-retry `_,
+   `autosar-a5-4-2-reinterpret-cast `_,
`boost-use-to-string `_, "Yes"
`bugprone-argument-comment `_, "Yes"
`bugprone-assert-side-effect `_,
@@ -328,6 +329,7 @@
 .. csv-table:: Aliases..
:header: "Name", "Redirect", "Offers fixes"
 
+   `autosar-a5-4-2-reinterpret-cast `_, `cppcoreguidelines-pro-type-reinterpret-cast `_,
`cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_,
`cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_,
`cert-dcl03-c `_, `misc-static-assert `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - autosar-a5-4-2-reinterpret-cast
+.. meta::
+   :http-equiv=refresh: 5;URL=cppcoreguidelines-pro-type-reinterpret-cast.html
+
+autosar-a5-4-2-reinterpret-cast
+===
+
+The autosar-a5-4-2-reinterpret-cast check is an alias, please see
+`cppcoreguidelines-pro-type-reinterpret-cast `_
+for more information.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,8 @@
 Improvements to clang-tidy
 --
 
+- New module: `autosar`, covering the AUTOSAR C++14 Coding Guidelines.
+
 - Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
@@ -103,6 +105,11 @@
 New check aliases
 ^
 
+- New alias :doc:`autosar-a5-4-2-reinterpret-cast
+  ` to
+  :doc:`cppcoreguidelines-pro-type-reinterpret-cast
+  ` was added.
+
 - New alias :doc:`cert-exp42-c
   ` to
   :doc:`bugprone-suspicious-memory-comparison
Index: clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
@@ -0,0 +1,27 @@
+set(LLVM_LINK_COMPONENTS
+  FrontendOpenMP
+  Support
+  )
+
+add_clang_library(clangTidyAutosarModule
+  AutosarTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUtils
+  clangTidyCppCoreGuid

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D112720#3094163 , @lebedev.ri 
wrote:

> As long as this blankedly breaks/regresses existing configs it's a 
> non-starter i think.

I see, thanks for the input!

I'm curious if there are any deprecation mechanisms for clang-tidy? Or can it 
only be updated in a backwards-compatible way? As seen in the other referenced 
patch, the backwards-compatible solution is to add yet another configurable 
parameter - `ExcludedHeaderFilterRegex`, which only contributes to more user 
confusion. Or keep calling the parameter `HeaderFilterRegex` and switch the 
implementation to something that is not a regex - even more confusion to the 
users.

Personally I think it would be good to have such mechanisms to be able to 
improve on existing design (not just add more functionality). It's impossible 
to predict the future so decisions that made total sense in the past might need 
to be revised later to adapt to user needs.


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

https://reviews.llvm.org/D112720

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

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Makes total sense, thanks for the suggestion!

I'll wait for more reviewers to see if this behavior is what we want at all. If 
so I can revert the breaking changes and introduce that option.


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

https://reviews.llvm.org/D112720

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> "C++14 Guidelines"? So it's always and definitely C++14 specific?

To my knowledge, AUTOSAR handed over the work to MISRA and the current 
direction is that MISRA will merge AUTOSAR C++14 and older MISRA revisions into 
a brand-new MISRA release, coming up 202x (The "x" is very unclear here, it 
could take years to get it in place). So I would be surprised if AUTOSAR 
released a new version after this. They way they are written they are 
targetting the use of C++14.

I suppose we can change the description in the future + add configuration to 
select whether to choose C++14 or something else if that ever comes up, but I 
think it's a bit premature at this point. Would you agree?

> What is the licencing approach of this guideline? Looking up with searchers 
> seems to turn up a few PDFs which say `--- AUTOSAR CONFIDENTIAL ---`, yet 
> they are up and out on the official-looking website.

Yes, that's strange. The disclaimer says the following:

> The material contained in this work is protected by copyright and other types 
> of
>  intellectual property rights. The commercial exploitation of the material 
> contained in
>  this work requires a license to such intellectual property rights.
>  This work may be utilized or reproduced without any modification, in any 
> form or by
>  any means, for informational purposes only. For any other purpose, no part 
> of the
>  work may be utilized or reproduced, in any form or by any means, without 
> permission
>  in writing from the publisher.

I'm not a lawyer for I think we can be sure that this is not a commercial use 
case? I guess if we were a company selling an AUTOSAR C++14-compliant static 
analyzer then we would need a license.


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

https://reviews.llvm.org/D112730

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 383259.
carlosgalvezp added a comment.

Removed unnecessary dependency in CMakeLists.txt


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

https://reviews.llvm.org/D112730

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/autosar/AutosarTidyModule.cpp
  clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-reinterpret-cast.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast,autosar-a5-2-4-reinterpret-cast %t
 
 int i = 0;
 void *j;
 void f() { j = reinterpret_cast(i); }
-// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [autosar-a5-2-4-reinterpret-cast,cppcoreguidelines-pro-type-reinterpret-cast]
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -60,6 +60,7 @@
 ``abseil-``Checks related to Abseil library.
 ``altera-``Checks related to OpenCL programming for FPGAs.
 ``android-``   Checks related to Android.
+``autosar-``   Checks related to AUTOSAR C++14 Coding Guidelines.
 ``boost-`` Checks related to Boost library.
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
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
@@ -50,6 +50,7 @@
`android-cloexec-pipe2 `_,
`android-cloexec-socket `_,
`android-comparison-in-temp-failure-retry `_,
+   `autosar-a5-4-2-reinterpret-cast `_,
`boost-use-to-string `_, "Yes"
`bugprone-argument-comment `_, "Yes"
`bugprone-assert-side-effect `_,
@@ -328,6 +329,7 @@
 .. csv-table:: Aliases..
:header: "Name", "Redirect", "Offers fixes"
 
+   `autosar-a5-4-2-reinterpret-cast `_, `cppcoreguidelines-pro-type-reinterpret-cast `_,
`cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_,
`cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_,
`cert-dcl03-c `_, `misc-static-assert `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/autosar-a5-4-2-reinterpret-cast.rst
@@ -0,0 +1,10 @@
+.. title:: clang-tidy - autosar-a5-4-2-reinterpret-cast
+.. meta::
+   :http-equiv=refresh: 5;URL=cppcoreguidelines-pro-type-reinterpret-cast.html
+
+autosar-a5-4-2-reinterpret-cast
+===
+
+The autosar-a5-4-2-reinterpret-cast check is an alias, please see
+`cppcoreguidelines-pro-type-reinterpret-cast `_
+for more information.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,8 @@
 Improvements to clang-tidy
 --
 
+- New module: `autosar`, covering the AUTOSAR C++14 Coding Guidelines.
+
 - Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
@@ -103,6 +105,11 @@
 New check aliases
 ^
 
+- New alias :doc:`autosar-a5-4-2-reinterpret-cast
+  ` to
+  :doc:`cppcoreguidelines-pro-type-reinterpret-cast
+  ` was added.
+
 - New alias :doc:`cert-exp42-c
   ` to
   :doc:`bugprone-suspicious-memory-comparison
Index: clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/autosar/CMakeLists.txt
@@ -0,0 +1,26 @@
+set(LLVM_LINK_COMPONENTS
+  FrontendOpenMP
+  Support
+  )
+
+add_clang_library(clangTidyAutosarModule
+  AutosarTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUt

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Also, I've sent a mail to AUTOSAR directly asking for consent, so there's no 
doubt.


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

https://reviews.llvm.org/D112730

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> people DO incorporate clang-tidy into their own commercial offerings

Gna, that //can// be a problem. I wonder if in that case it would be possible 
to add a few lines into the `LLVM Exceptions` part of the license. If it's too 
much of a hassle I guess I'll need to drop it and continue on a local fork 
unfortunately.

It's interesting however that this fork has implemented quite a few AUTOSAR 
checks keeping the existing LLVM license:
https://github.com/Bareflank/llvm-project/blob/bsl-tidy


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

https://reviews.llvm.org/D112730

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D112730#3097091 , @tstellar wrote:

> @carlosgalvezp Did you write this patch or did you get it from someone else?

@tstellar I wrote it myself (following the existing pattern for the other 
modules in the llvm-project repo).

Just to clarify, I mentioned the above repo to point out about the license, but 
I don't plan


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

https://reviews.llvm.org/D112730

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

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

[Carlos Galvez via Phabricator via cfe-commits]

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

Looks great, thanks for fixing! I'm surprised we don't have a CI bot running 
these checks post-merge?

Since this is my first review it's probably good to wait for other reviewers 
before merging in case I missed something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112864

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

[PATCH] D112720: [clang-tidy] Use globs in HeaderFilter

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

So after some thoughts, I came up with the following plan:

- This patch adds: `HeaderFilter`, `HeaderFilterMode`. `HeaderFilter` is 
intended to replace `HeaderFilterRegex` and we inform that `HeaderFilterRegex` 
is deprecated and will be removed in 2 releases. However during the transition 
period, both `HeaderFilter` and `HeaderFilterRegex` are accepted. 
`HeaderFilterMode` can take values: `regex` (default) or `glob`, and describes 
the behavior of `HeaderFilter` or `HeaderFilterRegex`. This allows people to 
use globs, while not breaking existing functionality.

- After 2 releases, we remove `HeaderFilterRegex` and set `HeaderFilterMode` as 
default to `glob`. We communicate that `HeaderFilterMode` is deprecated and 
will be removed in 2 releases.

- After (another) 2 releases, we remove `HeaderFilterMode`. The result is then 
what this patch looks like it is right now. Do we need to wait 2 more releases 
to remove `HeaderFilterMode` or can we do it already in the above step?

Does that sound good? Looking forward to your feedback.


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

https://reviews.llvm.org/D112720

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

[PATCH] D112913: Misleading bidirectional detection

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp requested changes to this revision.
carlosgalvezp added a comment.
This revision now requires changes to proceed.

Nice addition! Please add this check to the documentation (list of available 
checks + individual page with the documentation for this check), plus mention 
in the clang-tidy release notes. The same applies to the other related patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112913

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

[PATCH] D112913: Misleading bidirectional detection

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:61
+CheckFactories.registerCheck(
+"misc-misleading-bidirectional");
   }

Nit: please keep alphabetical order in the list of jobs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112913

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

[PATCH] D112914: Misleading identifier detection

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp requested changes to this revision.
carlosgalvezp added a comment.
This revision now requires changes to proceed.

- Add check to list of checks: 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/clang-tidy/checks/list.rst
- Mention check in the Release Notes: 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/ReleaseNotes.rst


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

https://reviews.llvm.org/D112914

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

[PATCH] D112914: Misleading identifier detection

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Looks good, I guess the license issue still needs to be sorted out?




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst:6
+
+Finds identifiers that contain Unicode characetrs with right-to-left direction,
+which can be confusing as they may change the understanding of a whole 
statement

characters



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-misleading-identifier.rst:10
+
+An exemple of such misleading code follows:
+

example


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

https://reviews.llvm.org/D112914

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

[PATCH] D112914: Misleading identifier detection

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp resigned from this revision.
carlosgalvezp added a comment.
This revision is now accepted and ready to land.

Ok! I don't really know what applies when you take //part// of a file, so I'll 
leave that up to people who know. I don't know how to remove the "Requested 
changes" from here so I'll just remove myself from reviewer.

PS: I don't know what AKAIU means, nor can I find the answer in Google :)


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

https://reviews.llvm.org/D112914

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

[PATCH] D113249: Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: asavonic, dexonsmith, hiraditya, yaxunl, jholewinski.
carlosgalvezp requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Following the pattern used for 11.4:
https://github.com/llvm/llvm-project/commit/49d982d8c6e01b6f8e4f173ed6325beab08b


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113249

Files:
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  llvm/docs/CompileCudaWithLLVM.rst
  llvm/lib/Target/NVPTX/NVPTX.td

Index: llvm/lib/Target/NVPTX/NVPTX.td
===
--- llvm/lib/Target/NVPTX/NVPTX.td
+++ llvm/lib/Target/NVPTX/NVPTX.td
@@ -93,6 +93,8 @@
  "Use PTX version 7.3">;
 def PTX74 : SubtargetFeature<"ptx74", "PTXVersion", "74",
  "Use PTX version 7.4">;
+def PTX75 : SubtargetFeature<"ptx75", "PTXVersion", "75",
+ "Use PTX version 7.5">;
 
 //===--===//
 // NVPTX supported processors.
Index: llvm/docs/CompileCudaWithLLVM.rst
===
--- llvm/docs/CompileCudaWithLLVM.rst
+++ llvm/docs/CompileCudaWithLLVM.rst
@@ -23,8 +23,8 @@
 -
 
 CUDA is supported since llvm 3.9. Clang currently supports CUDA 7.0 through
-10.1. If clang detects a newer CUDA version, it will issue a warning and will
-attempt to use detected CUDA SDK it as if it were CUDA-10.1.
+11.5. If clang detects a newer CUDA version, it will issue a warning and will
+attempt to use detected CUDA SDK it as if it were CUDA 11.5.
 
 Before you build CUDA code, you'll need to have installed the CUDA SDK.  See
 `NVIDIA's CUDA installation guide
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -65,6 +65,8 @@
 return CudaVersion::CUDA_113;
   if (raw_version < 11050)
 return CudaVersion::CUDA_114;
+  if (raw_version < 11060)
+return CudaVersion::CUDA_115;
   return CudaVersion::NEW;
 }
 
@@ -707,6 +709,7 @@
   case CudaVersion::CUDA_##CUDA_VER:   \
 PtxFeature = "+ptx" #PTX_VER;  \
 break;
+CASE_CUDA_VERSION(115, 75);
 CASE_CUDA_VERSION(114, 74);
 CASE_CUDA_VERSION(113, 73);
 CASE_CUDA_VERSION(112, 72);
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -44,6 +44,7 @@
 if (!Feature.startswith("+ptx"))
   continue;
 PTXVersion = llvm::StringSwitch(Feature)
+ .Case("+ptx75", 75)
  .Case("+ptx74", 74)
  .Case("+ptx73", 73)
  .Case("+ptx72", 72)
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -40,6 +40,8 @@
 return "11.3";
   case CudaVersion::CUDA_114:
 return "11.4";
+  case CudaVersion::CUDA_115:
+return "11.5";
   case CudaVersion::NEW:
 return "";
   }
@@ -62,6 +64,7 @@
   .Case("11.2", CudaVersion::CUDA_112)
   .Case("11.3", CudaVersion::CUDA_113)
   .Case("11.4", CudaVersion::CUDA_114)
+  .Case("11.5", CudaVersion::CUDA_115)
   .Default(CudaVersion::UNKNOWN);
 }
 
@@ -241,6 +244,8 @@
 return CudaVersion::CUDA_113;
   case 114:
 return CudaVersion::CUDA_114;
+  case 115:
+return CudaVersion::CUDA_115;
   default:
 return CudaVersion::UNKNOWN;
   }
Index: clang/include/clang/Basic/Cuda.h
===
--- clang/include/clang/Basic/Cuda.h
+++ clang/include/clang/Basic/Cuda.h
@@ -33,9 +33,10 @@
   CUDA_112,
   CUDA_113,
   CUDA_114,
-  FULLY_SUPPORTED = CUDA_114,
+  CUDA_115,
+  FULLY_SUPPORTED = CUDA_115,
   PARTIALLY_SUPPORTED =
-  CUDA_114, // Partially supported. Proceed with a warning.
+  CUDA_115, // Partially supported. Proceed with a warning.
   NEW = 1,  // Too new. Issue a warning, but allow using it.
 };
 const char *CudaVersionToString(CudaVersion V);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D113249#3110954 , @Hahnfeld wrote:

> I'm not sure if it's actually correct to advertise full support for CUDA 
> 11.5, but I didn't look into exact changes since 11.4

Good point. I was confused about the fact that 11.4 is both `FULLY_SUPPORTED` 
and `PARTIALLY_SUPPORTED`, so I thought to just follow the existing pattern. I 
didn't find any extra tests added for the bump 11.2 -> 11.4. Do we have 
infrastructure in place to test this, or how does it work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113249

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

[PATCH] D112913: Misleading bidirectional detection

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:105
+
+  Inspect string literal and comments for unterminated bidirectional Unicode
+  characters.

Nit: Inspects


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

https://reviews.llvm.org/D112913

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

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 385284.
carlosgalvezp added a comment.

Updated BuilintsNVPTX.def


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

https://reviews.llvm.org/D113249

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  llvm/docs/CompileCudaWithLLVM.rst
  llvm/lib/Target/NVPTX/NVPTX.td

Index: llvm/lib/Target/NVPTX/NVPTX.td
===
--- llvm/lib/Target/NVPTX/NVPTX.td
+++ llvm/lib/Target/NVPTX/NVPTX.td
@@ -93,6 +93,8 @@
  "Use PTX version 7.3">;
 def PTX74 : SubtargetFeature<"ptx74", "PTXVersion", "74",
  "Use PTX version 7.4">;
+def PTX75 : SubtargetFeature<"ptx75", "PTXVersion", "75",
+ "Use PTX version 7.5">;
 
 //===--===//
 // NVPTX supported processors.
Index: llvm/docs/CompileCudaWithLLVM.rst
===
--- llvm/docs/CompileCudaWithLLVM.rst
+++ llvm/docs/CompileCudaWithLLVM.rst
@@ -23,8 +23,8 @@
 -
 
 CUDA is supported since llvm 3.9. Clang currently supports CUDA 7.0 through
-10.1. If clang detects a newer CUDA version, it will issue a warning and will
-attempt to use detected CUDA SDK it as if it were CUDA-10.1.
+11.5. If clang detects a newer CUDA version, it will issue a warning and will
+attempt to use detected CUDA SDK it as if it were CUDA 11.5.
 
 Before you build CUDA code, you'll need to have installed the CUDA SDK.  See
 `NVIDIA's CUDA installation guide
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -65,6 +65,8 @@
 return CudaVersion::CUDA_113;
   if (raw_version < 11050)
 return CudaVersion::CUDA_114;
+  if (raw_version < 11060)
+return CudaVersion::CUDA_115;
   return CudaVersion::NEW;
 }
 
@@ -707,6 +709,7 @@
   case CudaVersion::CUDA_##CUDA_VER:   \
 PtxFeature = "+ptx" #PTX_VER;  \
 break;
+CASE_CUDA_VERSION(115, 75);
 CASE_CUDA_VERSION(114, 74);
 CASE_CUDA_VERSION(113, 73);
 CASE_CUDA_VERSION(112, 72);
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -44,6 +44,7 @@
 if (!Feature.startswith("+ptx"))
   continue;
 PTXVersion = llvm::StringSwitch(Feature)
+ .Case("+ptx75", 75)
  .Case("+ptx74", 74)
  .Case("+ptx73", 73)
  .Case("+ptx72", 72)
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -40,6 +40,8 @@
 return "11.3";
   case CudaVersion::CUDA_114:
 return "11.4";
+  case CudaVersion::CUDA_115:
+return "11.5";
   case CudaVersion::NEW:
 return "";
   }
@@ -62,6 +64,7 @@
   .Case("11.2", CudaVersion::CUDA_112)
   .Case("11.3", CudaVersion::CUDA_113)
   .Case("11.4", CudaVersion::CUDA_114)
+  .Case("11.5", CudaVersion::CUDA_115)
   .Default(CudaVersion::UNKNOWN);
 }
 
@@ -241,6 +244,8 @@
 return CudaVersion::CUDA_113;
   case 114:
 return CudaVersion::CUDA_114;
+  case 115:
+return CudaVersion::CUDA_115;
   default:
 return CudaVersion::UNKNOWN;
   }
Index: clang/include/clang/Basic/Cuda.h
===
--- clang/include/clang/Basic/Cuda.h
+++ clang/include/clang/Basic/Cuda.h
@@ -33,9 +33,10 @@
   CUDA_112,
   CUDA_113,
   CUDA_114,
-  FULLY_SUPPORTED = CUDA_114,
+  CUDA_115,
+  FULLY_SUPPORTED = CUDA_115,
   PARTIALLY_SUPPORTED =
-  CUDA_114, // Partially supported. Proceed with a warning.
+  CUDA_115, // Partially supported. Proceed with a warning.
   NEW = 1,  // Too new. Issue a warning, but allow using it.
 };
 const char *CudaVersionToString(CudaVersion V);
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -41,7 +41,9 @@
 #pragma push_macro("PTX72")
 #pragma push_macro("PTX73")
 #pragma push_macro("PTX74")
-#define PTX74 "ptx74"
+#pragma push_macro("PTX75")
+#define PTX75 "ptx75"
+#define PTX74 "ptx74|" PTX75
 #define PTX73 "ptx73|" PTX74
 #define PTX72 "ptx72|" PTX73
 #define PTX71 "ptx71|" PTX72
@@ -827,3 +829,4 @@
 #pragma pop_macro("PTX72")
 #pragma pop_macro("PTX73")
 #pragma pop_macro("PTX74")
+#pragma pop_macro("PTX75")
___

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> - The driver needs to enable ptx75 when it constructs cc1 command line in 
> clang/lib/Driver/ToolChains/Cuda.cpp

@tra Haven't I already done it in line 712? Or where should I enable it?


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

https://reviews.llvm.org/D113249

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

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 385286.
carlosgalvezp added a comment.

Update release notes


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

https://reviews.llvm.org/D113249

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  llvm/docs/CompileCudaWithLLVM.rst
  llvm/lib/Target/NVPTX/NVPTX.td

Index: llvm/lib/Target/NVPTX/NVPTX.td
===
--- llvm/lib/Target/NVPTX/NVPTX.td
+++ llvm/lib/Target/NVPTX/NVPTX.td
@@ -93,6 +93,8 @@
  "Use PTX version 7.3">;
 def PTX74 : SubtargetFeature<"ptx74", "PTXVersion", "74",
  "Use PTX version 7.4">;
+def PTX75 : SubtargetFeature<"ptx75", "PTXVersion", "75",
+ "Use PTX version 7.5">;
 
 //===--===//
 // NVPTX supported processors.
Index: llvm/docs/CompileCudaWithLLVM.rst
===
--- llvm/docs/CompileCudaWithLLVM.rst
+++ llvm/docs/CompileCudaWithLLVM.rst
@@ -23,8 +23,8 @@
 -
 
 CUDA is supported since llvm 3.9. Clang currently supports CUDA 7.0 through
-10.1. If clang detects a newer CUDA version, it will issue a warning and will
-attempt to use detected CUDA SDK it as if it were CUDA-10.1.
+11.5. If clang detects a newer CUDA version, it will issue a warning and will
+attempt to use detected CUDA SDK it as if it were CUDA 11.5.
 
 Before you build CUDA code, you'll need to have installed the CUDA SDK.  See
 `NVIDIA's CUDA installation guide
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -65,6 +65,8 @@
 return CudaVersion::CUDA_113;
   if (raw_version < 11050)
 return CudaVersion::CUDA_114;
+  if (raw_version < 11060)
+return CudaVersion::CUDA_115;
   return CudaVersion::NEW;
 }
 
@@ -707,6 +709,7 @@
   case CudaVersion::CUDA_##CUDA_VER:   \
 PtxFeature = "+ptx" #PTX_VER;  \
 break;
+CASE_CUDA_VERSION(115, 75);
 CASE_CUDA_VERSION(114, 74);
 CASE_CUDA_VERSION(113, 73);
 CASE_CUDA_VERSION(112, 72);
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -44,6 +44,7 @@
 if (!Feature.startswith("+ptx"))
   continue;
 PTXVersion = llvm::StringSwitch(Feature)
+ .Case("+ptx75", 75)
  .Case("+ptx74", 74)
  .Case("+ptx73", 73)
  .Case("+ptx72", 72)
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -40,6 +40,8 @@
 return "11.3";
   case CudaVersion::CUDA_114:
 return "11.4";
+  case CudaVersion::CUDA_115:
+return "11.5";
   case CudaVersion::NEW:
 return "";
   }
@@ -62,6 +64,7 @@
   .Case("11.2", CudaVersion::CUDA_112)
   .Case("11.3", CudaVersion::CUDA_113)
   .Case("11.4", CudaVersion::CUDA_114)
+  .Case("11.5", CudaVersion::CUDA_115)
   .Default(CudaVersion::UNKNOWN);
 }
 
@@ -241,6 +244,8 @@
 return CudaVersion::CUDA_113;
   case 114:
 return CudaVersion::CUDA_114;
+  case 115:
+return CudaVersion::CUDA_115;
   default:
 return CudaVersion::UNKNOWN;
   }
Index: clang/include/clang/Basic/Cuda.h
===
--- clang/include/clang/Basic/Cuda.h
+++ clang/include/clang/Basic/Cuda.h
@@ -33,9 +33,10 @@
   CUDA_112,
   CUDA_113,
   CUDA_114,
-  FULLY_SUPPORTED = CUDA_114,
+  CUDA_115,
+  FULLY_SUPPORTED = CUDA_115,
   PARTIALLY_SUPPORTED =
-  CUDA_114, // Partially supported. Proceed with a warning.
+  CUDA_115, // Partially supported. Proceed with a warning.
   NEW = 1,  // Too new. Issue a warning, but allow using it.
 };
 const char *CudaVersionToString(CudaVersion V);
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -41,7 +41,9 @@
 #pragma push_macro("PTX72")
 #pragma push_macro("PTX73")
 #pragma push_macro("PTX74")
-#define PTX74 "ptx74"
+#pragma push_macro("PTX75")
+#define PTX75 "ptx75"
+#define PTX74 "ptx74|" PTX75
 #define PTX73 "ptx73|" PTX74
 #define PTX72 "ptx72|" PTX73
 #define PTX71 "ptx71|" PTX72
@@ -827,3 +829,4 @@
 #pragma pop_macro("PTX72")
 #pragma pop_macro("PTX73")
 #pragma pop_macro("PTX74")
+#pragma pop_macro("PTX75")

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
carlosgalvezp added reviewers: aaron.ballman, whisperity.
Herald added subscribers: rnkovacs, xazax.hun.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Currently it's hidden inside ClangTidyDiagnosticConsumer,
so it's hard to know it exists.

Given that there are multiple uses of globs in clang-tidy,
it makes sense to have this classes publicly available
for other use cases that might benefit from it.

Also, add unit test by converting the existing tests
for GlobList into typed tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113422

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/GlobList.cpp
  clang-tools-extra/clang-tidy/GlobList.h
  clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/GlobListTest.cpp
@@ -4,15 +4,20 @@
 namespace clang {
 namespace tidy {
 
-TEST(GlobList, Empty) {
-  GlobList Filter("");
+template  struct GlobListTest : public ::testing::Test {};
+
+using GlobListTypes = ::testing::Types;
+TYPED_TEST_SUITE(GlobListTest, GlobListTypes);
+
+TYPED_TEST(GlobListTest, Empty) {
+  TypeParam Filter("");
 
   EXPECT_TRUE(Filter.contains(""));
   EXPECT_FALSE(Filter.contains("aaa"));
 }
 
-TEST(GlobList, Nothing) {
-  GlobList Filter("-*");
+TYPED_TEST(GlobListTest, Nothing) {
+  TypeParam Filter("-*");
 
   EXPECT_FALSE(Filter.contains(""));
   EXPECT_FALSE(Filter.contains("a"));
@@ -21,8 +26,8 @@
   EXPECT_FALSE(Filter.contains("*"));
 }
 
-TEST(GlobList, Everything) {
-  GlobList Filter("*");
+TYPED_TEST(GlobListTest, Everything) {
+  TypeParam Filter("*");
 
   EXPECT_TRUE(Filter.contains(""));
   EXPECT_TRUE(Filter.contains(""));
@@ -31,8 +36,8 @@
   EXPECT_TRUE(Filter.contains("*"));
 }
 
-TEST(GlobList, OneSimplePattern) {
-  GlobList Filter("aaa");
+TYPED_TEST(GlobListTest, OneSimplePattern) {
+  TypeParam Filter("aaa");
 
   EXPECT_TRUE(Filter.contains("aaa"));
   EXPECT_FALSE(Filter.contains(""));
@@ -41,8 +46,8 @@
   EXPECT_FALSE(Filter.contains("bbb"));
 }
 
-TEST(GlobList, TwoSimplePatterns) {
-  GlobList Filter("aaa,bbb");
+TYPED_TEST(GlobListTest, TwoSimplePatterns) {
+  TypeParam Filter("aaa,bbb");
 
   EXPECT_TRUE(Filter.contains("aaa"));
   EXPECT_TRUE(Filter.contains("bbb"));
@@ -52,11 +57,11 @@
   EXPECT_FALSE(Filter.contains(""));
 }
 
-TEST(GlobList, PatternPriority) {
+TYPED_TEST(GlobListTest, PatternPriority) {
   // The last glob that matches the string decides whether that string is
   // included or excluded.
   {
-GlobList Filter("a*,-aaa");
+TypeParam Filter("a*,-aaa");
 
 EXPECT_FALSE(Filter.contains(""));
 EXPECT_TRUE(Filter.contains("a"));
@@ -65,7 +70,7 @@
 EXPECT_TRUE(Filter.contains(""));
   }
   {
-GlobList Filter("-aaa,a*");
+TypeParam Filter("-aaa,a*");
 
 EXPECT_FALSE(Filter.contains(""));
 EXPECT_TRUE(Filter.contains("a"));
@@ -75,15 +80,16 @@
   }
 }
 
-TEST(GlobList, WhitespacesAtBegin) {
-  GlobList Filter("-*,   a.b.*");
+TYPED_TEST(GlobListTest, WhitespacesAtBegin) {
+  TypeParam Filter("-*,   a.b.*");
 
   EXPECT_TRUE(Filter.contains("a.b.c"));
   EXPECT_FALSE(Filter.contains("b.c"));
 }
 
-TEST(GlobList, Complex) {
-  GlobList Filter("*,-a.*, -b.*, \r  \n  a.1.* ,-a.1.A.*,-..,-...,-..+,-*$, -*qwe* ");
+TYPED_TEST(GlobListTest, Complex) {
+  TypeParam Filter(
+  "*,-a.*, -b.*, \r  \n  a.1.* ,-a.1.A.*,-..,-...,-..+,-*$, -*qwe* ");
 
   EXPECT_TRUE(Filter.contains("aaa"));
   EXPECT_TRUE(Filter.contains("qqq"));
Index: clang-tools-extra/clang-tidy/GlobList.h
===
--- clang-tools-extra/clang-tidy/GlobList.h
+++ clang-tools-extra/clang-tidy/GlobList.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Regex.h"
 
@@ -39,7 +40,6 @@
   bool contains(StringRef S) const;
 
 private:
-
   struct GlobListItem {
 bool IsPositive;
 llvm::Regex Regex;
@@ -47,6 +47,22 @@
   SmallVector Items;
 };
 
+/// A \p GlobList that caches search results, so that search is performed only
+/// once for the same query.
+class CachedGlobList {
+public:
+  /// \see GlobList::GlobList
+  CachedGlobList(StringRef Globs, bool KeepNegativeGlobs = true);
+
+  /// \see GlobList::contains
+  bool contains(StringRef S);
+
+private:
+  GlobList Globs;
+  enum Tristate { None, Yes, No };
+  llvm::StringMap Cache;
+};
+
 } // end namespace tidy
 } // end namespace clang
 
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@Hahnfeld Are you satisfied with the replies to your questions? If so I can go 
ahead and merge.


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

https://reviews.llvm.org/D113249

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

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Awesome, thanks a lot for the reviews!


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

https://reviews.llvm.org/D113249

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

[PATCH] D113249: [CUDA] Bump CUDA version to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7ecec3f0f521: [CUDA] Bump supported CUDA version to 11.5 
(authored by carlosgalvezp).

Changed prior to commit:
  https://reviews.llvm.org/D113249?vs=385286&id=385717#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113249

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  llvm/docs/CompileCudaWithLLVM.rst
  llvm/lib/Target/NVPTX/NVPTX.td

Index: llvm/lib/Target/NVPTX/NVPTX.td
===
--- llvm/lib/Target/NVPTX/NVPTX.td
+++ llvm/lib/Target/NVPTX/NVPTX.td
@@ -93,6 +93,8 @@
  "Use PTX version 7.3">;
 def PTX74 : SubtargetFeature<"ptx74", "PTXVersion", "74",
  "Use PTX version 7.4">;
+def PTX75 : SubtargetFeature<"ptx75", "PTXVersion", "75",
+ "Use PTX version 7.5">;
 
 //===--===//
 // NVPTX supported processors.
Index: llvm/docs/CompileCudaWithLLVM.rst
===
--- llvm/docs/CompileCudaWithLLVM.rst
+++ llvm/docs/CompileCudaWithLLVM.rst
@@ -23,8 +23,8 @@
 -
 
 CUDA is supported since llvm 3.9. Clang currently supports CUDA 7.0 through
-10.1. If clang detects a newer CUDA version, it will issue a warning and will
-attempt to use detected CUDA SDK it as if it were CUDA-10.1.
+11.5. If clang detects a newer CUDA version, it will issue a warning and will
+attempt to use detected CUDA SDK it as if it were CUDA 11.5.
 
 Before you build CUDA code, you'll need to have installed the CUDA SDK.  See
 `NVIDIA's CUDA installation guide
Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -65,6 +65,8 @@
 return CudaVersion::CUDA_113;
   if (raw_version < 11050)
 return CudaVersion::CUDA_114;
+  if (raw_version < 11060)
+return CudaVersion::CUDA_115;
   return CudaVersion::NEW;
 }
 
@@ -707,6 +709,7 @@
   case CudaVersion::CUDA_##CUDA_VER:   \
 PtxFeature = "+ptx" #PTX_VER;  \
 break;
+CASE_CUDA_VERSION(115, 75);
 CASE_CUDA_VERSION(114, 74);
 CASE_CUDA_VERSION(113, 73);
 CASE_CUDA_VERSION(112, 72);
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -44,6 +44,7 @@
 if (!Feature.startswith("+ptx"))
   continue;
 PTXVersion = llvm::StringSwitch(Feature)
+ .Case("+ptx75", 75)
  .Case("+ptx74", 74)
  .Case("+ptx73", 73)
  .Case("+ptx72", 72)
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -40,6 +40,8 @@
 return "11.3";
   case CudaVersion::CUDA_114:
 return "11.4";
+  case CudaVersion::CUDA_115:
+return "11.5";
   case CudaVersion::NEW:
 return "";
   }
@@ -62,6 +64,7 @@
   .Case("11.2", CudaVersion::CUDA_112)
   .Case("11.3", CudaVersion::CUDA_113)
   .Case("11.4", CudaVersion::CUDA_114)
+  .Case("11.5", CudaVersion::CUDA_115)
   .Default(CudaVersion::UNKNOWN);
 }
 
@@ -241,6 +244,8 @@
 return CudaVersion::CUDA_113;
   case 114:
 return CudaVersion::CUDA_114;
+  case 115:
+return CudaVersion::CUDA_115;
   default:
 return CudaVersion::UNKNOWN;
   }
Index: clang/include/clang/Basic/Cuda.h
===
--- clang/include/clang/Basic/Cuda.h
+++ clang/include/clang/Basic/Cuda.h
@@ -33,9 +33,10 @@
   CUDA_112,
   CUDA_113,
   CUDA_114,
-  FULLY_SUPPORTED = CUDA_114,
+  CUDA_115,
+  FULLY_SUPPORTED = CUDA_115,
   PARTIALLY_SUPPORTED =
-  CUDA_114, // Partially supported. Proceed with a warning.
+  CUDA_115, // Partially supported. Proceed with a warning.
   NEW = 1,  // Too new. Issue a warning, but allow using it.
 };
 const char *CudaVersionToString(CudaVersion V);
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -41,7 +41,9 @@
 #pragma push_macro("PTX72")
 #pragma push_macro("PTX73")
 #pragma push_macro("PTX74")
-#define PTX74 "ptx74"
+#pragma push_macro("PTX75")
+#define PTX75 "ptx75"
+#define PTX74 "ptx74|" PTX75
 #define PTX73

[PATCH] D110155: [OpenCL] Allow optional __generic in __remove_address_space utility

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Hi! I believe this commit broke a number of CI jobs, would you be able to look 
into it?

https://lab.llvm.org/buildbot/#/builders/52/builds/12263


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110155

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

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Hmm, this sounds a bit hacky. I noticed a similar pattern in tests. I think it 
deteriorates readability a bit.

Can we make the detection of the tag more robust? Right now we check if a line 
contains NOLINTBEGIN/END. Why not check if it contains "// NOLINTBEGIN"? (// as 
part of the line).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> If it is to be robust with /* multiline comments */

Does it have to be? I don't recall that we have unit tests for that. I would 
personally restrict the usage to only one way to write the line. I think it's 
not hard for users to adapt to that, plus it encourage them to write more 
readable code.

But OK, since it can potentially be a large change let's leave it for another 
time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D112730#3116281 , @tstellar wrote:

> @carlosgalvezp The LLVM Foundation Board will conduct a legal reivew of this 
> patch.  Would you be able to share any information you have about the license 
> or usage restrictions for the AUTOSAR specification?

@tstellar Thanks for taking the time to review this, very much appreciated. 
Absolutely, here's what I know:

- The AUTOSAR C++14 guidelines can be obtained freely from here: 
https://www.autosar.org/fileadmin/user_upload/standards/adaptive/20-11/AUTOSAR_RS_CPP14Guidelines.pdf

  Note: this is a new version, compared to the one one could find via a Google 
search. It's still publicly available without any kind of paywall or 
registration needed. It can be searched here: 
https://www.autosar.org/nc/document-search/

- All pages of the pdf contain the header "AUTOSAR CONFIDENTIAL". I don't 
understand what that means legally, given that the document is public.

- The second page of the pdf contains a legal disclaimer, that claims the 
document shall be used "for information purposes only". For commercial usage, 
written permission from AUTOSAR must be obtained. I think it would be best if 
you look at the written statements directly, I might have missed important bits 
here.

- There exists a llvm-project fork that has implemented some of the checks: 
https://github.com/Bareflank/llvm-project/tree/bsl-tidy/clang-tools-extra/clang-tidy/bsl.
 They don't explicitly name Autosar anywhere in the code, but it's clear that 
they implement Autosar checks. In fact some commits refer to the rule number. 
The license of this fork is kept as the existing Apache 2.0 with LLVM 
Exceptions. I don't know if this was agreed with Autosar or simply the author 
didn't take enough consideration about the licensing aspects. I just want to 
mention that open-source Autosar checks already exist today under that license, 
whether it's a mistake or not.

- I have sent an email to AUTOSAR requesting their consent to implement 
open-source checks. Would you like me to CC you and other members of the Board 
into that mail? Who should I add?

- More restrictive guidelines, like MISRA, do allow open-source checkers, as 
long as only the rule number (not the rule text) is displayed.

I believe that's all I know, I hope it helps in reviewing this issue. Let me 
know if there are more questions or anything is unclear!


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

https://reviews.llvm.org/D112730

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

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Great! I'd be open to supporting `/*` as well if people really need it. But so 
far that use case is not documented nor tested, so I think we shouldn't add new 
functionality if it's not needed. It can be added in the future if someone has 
a compelling case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D113518: [clang] Create delegating constructors even in templates

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp requested changes to this revision.
carlosgalvezp added a comment.
This revision now requires changes to proceed.

Amazing, thanks a lot for fixing this long-standing issue! I have a couple 
comments. I'm not familiar at all with the Sema part so someone that knows 
should look at it :)




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:374
 {
   PositiveSelfInitialization() : PositiveSelfInitialization() {}
 };

Not really sure what this test is meant to do. Why would it call the destructor 
of the own class in it's own constructor? Looks very strange to me.

If anything, the constructor should call the constructor of the base:

PositiveSelfInitialization() : NegativeAggregateType()

What do you think?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:555
+// Check that a delegating constructor in a template does not trigger false 
positives (PR#37902).
+template 
+struct TemplateWithDelegatingCtor {

typename T?

Not sure if it makes a difference



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:559
+  TemplateWithDelegatingCtor() : X{} {};
+  TemplateWithDelegatingCtor(int) : TemplateWithDelegatingCtor(){};
+};

Could you add another test where X is initialized via NSDMI instead of in the 
constructor initializer list?

int X{};
TemplateWithDelegatingCtor() = default;
TemplateWithDelegatingCtor(int) : TemplateWithDelegatingCtor() {}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113518

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

[PATCH] D113518: [clang] Create delegating constructors even in templates

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Also, what's with the pre-merge test, is it related?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113518

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

[PATCH] D113518: [clang] Create delegating constructors even in templates

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:374
 {
   PositiveSelfInitialization() : PositiveSelfInitialization() {}
 };

fwolff wrote:
> carlosgalvezp wrote:
> > Not really sure what this test is meant to do. Why would it call the 
> > destructor of the own class in it's own constructor? Looks very strange to 
> > me.
> > 
> > If anything, the constructor should call the constructor of the base:
> > 
> > PositiveSelfInitialization() : NegativeAggregateType()
> > 
> > What do you think?
> The comment above talks about a "pathological template", so my guess is that 
> this checks that clang-tidy doesn't crash for this input. The only reason why 
> I had to touch this test at all is that the constructor is now treated as a 
> delegating constructor, which suppresses the warning.
Hmm, I see. I would like to make sure we still catch the failure mode "this 
constructor does not initialize these base classes" for class templates.

I don't see such test existing (only for non-template classes), maybe you can 
add that too?


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

https://reviews.llvm.org/D113518

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

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I was a bit confused as well about the ClangSA support in clang-tidy. What's 
the current status? Is clang-tidy running *all* checks from StaticAnalyzer?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64454

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

[PATCH] D113518: [clang] Create delegating constructors even in templates

[Carlos Galvez via Phabricator via cfe-commits]

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

Looks great, thanks! I'm not comfortable reviewing the Sema part, maybe someone 
else can have a look?


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

https://reviews.llvm.org/D113518

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