[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> I still think even if we can subset this, for whatever we're going to turn 
> into a hard error, it should be a warning-as-error in system headers first 
> for at least a release. (so perhaps the transition should look like: null (no 
> diagnostic) -> warning -> warning-default-to-error -> 
> warning-default-to-error-even-in-system-headers -> hard error)

@dblaikie Thanks for the input, I believe I achieve the behavior you propose 
here, feel free to review! https://github.com/llvm/llvm-project/pull/67528


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

https://reviews.llvm.org/D150226

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

@shafik @aaron.ballman @dblaikie

Hello! Just wanted to check if there's any blockers for merging this patch? We 
are now on Clang 18, i.e. 2 releases after the warning was introduced, so IMO I 
believe it's a good time to turn it into a hard error and test it in the wild.

I read concerns about breaking code. Technically, UB is only invoked in C++17 
and onwards (before, it's only unspecified behavior) - could a solution to 
mitigate risk/break less code be to only enable this hard error in C++17? This 
way, only people who use a relatively new C++ Standard and compiler would get 
the error.

I also wonder what is the way forward with respect to code reviews, since 
Phabricator is deprecated. @shafik do you intend to continue here, or will you 
duplicate this into a Github PR?

Happy to help if there's anything I can do! Thanks for the great work :)

I'm happy to help


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

https://reviews.llvm.org/D150226

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

[PATCH] D159436: [clang-tidy] Add support for optional parameters in config.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp:18
+// RUN:   -config='{CheckOptions: { \
+// RUN: bugprone-easily-swappable-parameters.MinimumLength: "false", \
+// RUN:  }}' --

What if:

- "MinimumLength" is a boolean.
- It's default value (if not specified) is True.
- And a user wants to set it as "False" here

Wouldn't that cause problems?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159436

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

[PATCH] D159436: [clang-tidy] Add support for optional parameters in config.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the patch! I must admit I don't fully understand what problem it 
solves, i.e. parameters are already optional today (one just simply doesn't 
specify them in the config file). Why would we want to explicitly spell out 
parameters with some default value that doesn't correspond to what actually 
gets applied in the check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159436

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

[PATCH] D158486: [clang-tidy] Ignore used special-members in modernize-use-equals-delete

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, thanks for fixing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158486

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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56644

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

[PATCH] D158929: [clang-tidy] Add exit code support to clang-tidy-diff.py

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158929

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

[PATCH] D157374: [clang-tidy] Ignore decltype in misc-redundant-expression

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157374

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

[PATCH] D157326: [clang-tidy] Fix handling of out-of-line functions in readability-static-accessed-through-instance

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157326

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

[PATCH] D157239: [clang-tidy] Implement bugprone-incorrect-enable-if

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Do you have plans to also detect the bugprone scenario described in the `Notes` 
here?

https://en.cppreference.com/w/cpp/types/enable_if

No need to have it in this patch, but would be good to keep it in mind if we 
want to add it in the future (preferably) or create a separate check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157239

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

[PATCH] D157185: [clang-tidy] Fix false-positives in performanc-noexcept-swap

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.cpp:31
+  // Match function with 2 arguments, both are non-const references to same 
type
+  // and return void void swap(Type&, Type&)
+  auto FunctionMatcher = allOf(

Duplicate void, remove



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-swap.cpp:215
+void swap(int&);
+static void swap(int&, int&);
+  };

Could we also test:

`friend void swap(T&, T&)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157185

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

[PATCH] D157181: [clang-tidy] Re-add cppcoreguidelines-macro-to-enum alias

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D157181#4563170 , @PiotrZSL wrote:

> In D157181#4563147 , @carlosgalvezp 
> wrote:
>
>> Strange, it seems the alias was never fully added?
>> https://reviews.llvm.org/D117522
>
> Looks like, still I dont know what to do with release notes, add alias 
> "again" there, dont change it, or mention that there were bug.

I think adding it again is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157181

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

[PATCH] D157190: [clang-tidy] Fixed false-negative in readability-identifier-naming

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, minor question that can be fixed post-review unless you want to discuss 
further!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp:716
+// CHECK-FIXES: {{^}}class CClassWithForwardDecl {
+class class_with_forward_decl {
+  int __value;

Nit: shouldn't we use also `struct` here? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157190

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

[PATCH] D157185: [clang-tidy] Fix false-positives in performanc-noexcept-swap

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.cpp:22
   Finder->addMatcher(
-  functionDecl(unless(isDeleted()), 
hasName("swap")).bind(BindFuncDeclName),
+  functionDecl(
+  unless(isDeleted()), hasName("swap"),

Would be good to add a small comment summarizing what signatures this block of 
code matches, for easier readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157185

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

[PATCH] D157180: [clang-tidy] Exclude class/struct scope variables from cppcoreguidelines-avoid-non-const-global-variables

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157180

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

[PATCH] D157178: [clang-tidy] Fix inline namespaces in llvm-namespace-comment

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157178

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

[PATCH] D145138: [clang-tidy] Implement FixIts for C arrays

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D145138#4561799 , @PiotrZSL wrote:

> Test are failing, and I do not think that converting c-strings into 
> std::array is a good idea

+1, they should typically be `char const*` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145138

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

[PATCH] D157181: [clang-tidy] Re-add cppcoreguidelines-macro-to-enum alias

[Carlos Galvez via Phabricator via cfe-commits]

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

Strange, it seems the alias was never fully added?
https://reviews.llvm.org/D117522


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157181

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

[PATCH] D157182: [clang-tidy][NFC] Update documentation for hicpp-avoid-goto

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157182

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

[PATCH] D156624: [clang-tidy] Access checks not done classes derived of std::array

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

What about the use case of privately inheriting `std::array`, and overriding 
the `operator[]` there with bounds check? Would it be possible to check only 
_public_ inheritance?


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

https://reviews.llvm.org/D156624

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

[PATCH] D156608: [clang-tidy] Do not warn on macros starting with underscore and lowercase letter in bugprone-reserved-identifier

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2992d084774f: [clang-tidy] Do not warn on macros starting 
with underscore and lowercase… (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156608

Files:
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
@@ -45,7 +45,7 @@
 
 #define __macro(m) int m = 0
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '__macro', which is a reserved identifier [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define macro(m) int m = 0{{$}}
+// CHECK-FIXES: {{^}}#define _macro(m) int m = 0{{$}}
 
 namespace __ns {
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration uses identifier '__ns', which is a reserved identifier [bugprone-reserved-identifier]
@@ -115,9 +115,9 @@
 
 //
 
+// OK, this rule does not apply to macros:
+// https://github.com/llvm/llvm-project/issues/64130#issuecomment-1655751676
 #define _macro(m) int m = 0
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_macro', which is reserved in the global namespace [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define macro(m) int m = 0{{$}}
 
 namespace _ns {
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration uses identifier '_ns', which is reserved in the global namespace [bugprone-reserved-identifier]
@@ -171,10 +171,9 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: declaration uses identifier '_', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]
 // CHECK-FIXES: {{^}}int _;{{$}}
 
+// This should not trigger a warning
 // https://github.com/llvm/llvm-project/issues/52895
 #define _5_kmph_rpm 459
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_5_kmph_rpm', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define _5_kmph_rpm 459{{$}}
 
 // these should pass
 #define MACRO(m) int m = 0
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -140,6 +140,10 @@
 Changes in existing checks
 ^^
 
+- Fixed bug in :doc:`bugprone-reserved-identifier
+  `, so that it does not warn
+  on macros starting with underscore and lowercase letter.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
@@ -102,13 +102,15 @@
 }
 
 static bool startsWithUnderscoreInGlobalNamespace(StringRef Name,
-  bool IsInGlobalNamespace) {
-  return IsInGlobalNamespace && Name.size() >= 1 && Name[0] == '_';
+  bool IsInGlobalNamespace,
+  bool IsMacro) {
+  return !IsMacro && IsInGlobalNamespace && Name.size() >= 1 && Name[0] == '_';
 }
 
 static std::optional
-getUnderscoreGlobalNamespaceFixup(StringRef Name, bool IsInGlobalNamespace) {
-  if (startsWithUnderscoreInGlobalNamespace(Name, IsInGlobalNamespace))
+getUnderscoreGlobalNamespaceFixup(StringRef Name, bool IsInGlobalNamespace,
+  bool IsMacro) {
+  if (startsWithUnderscoreInGlobalNamespace(Name, IsInGlobalNamespace, IsMacro))
 return std::string(Name.drop_front(1));
   return std::nullopt;
 }
@@ -123,7 +125,7 @@
 }
 
 static std::optional
-getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace,
+getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace, bool IsMacro,
const LangOptions , bool Invert,
ArrayRef AllowedIdentifiers) {
   assert(!Name.empty());
@@ -158,15 +160,16 @@
   AppendFailure(DoubleUnderscoreTag, std::move(*Fixup));
 if (auto Fixup = getUnderscoreCapitalFixup(InProgressFixup()))
   AppendFailure(UnderscoreCapitalTag, std::move(*Fixup));
-if (auto Fixup = getUnderscoreGlobalNamespaceFixup(InProgressFixup(),
-   IsInGlobalNamespace))
+if (auto Fixup = getUnderscoreGlobalNamespaceFixup(
+InProgressFixup(), 

[PATCH] D156608: [clang-tidy] Do not warn on macros starting with underscore and lowercase letter in bugprone-reserved-identifier

[Carlos Galvez via Phabricator via cfe-commits]

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

Remove extra newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156608

Files:
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
@@ -45,7 +45,7 @@
 
 #define __macro(m) int m = 0
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '__macro', which is a reserved identifier [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define macro(m) int m = 0{{$}}
+// CHECK-FIXES: {{^}}#define _macro(m) int m = 0{{$}}
 
 namespace __ns {
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration uses identifier '__ns', which is a reserved identifier [bugprone-reserved-identifier]
@@ -115,9 +115,9 @@
 
 //
 
+// OK, this rule does not apply to macros:
+// https://github.com/llvm/llvm-project/issues/64130#issuecomment-1655751676
 #define _macro(m) int m = 0
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_macro', which is reserved in the global namespace [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define macro(m) int m = 0{{$}}
 
 namespace _ns {
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration uses identifier '_ns', which is reserved in the global namespace [bugprone-reserved-identifier]
@@ -171,10 +171,9 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: declaration uses identifier '_', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]
 // CHECK-FIXES: {{^}}int _;{{$}}
 
+// This should not trigger a warning
 // https://github.com/llvm/llvm-project/issues/52895
 #define _5_kmph_rpm 459
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_5_kmph_rpm', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define _5_kmph_rpm 459{{$}}
 
 // these should pass
 #define MACRO(m) int m = 0
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -140,6 +140,10 @@
 Changes in existing checks
 ^^
 
+- Fixed bug in :doc:`bugprone-reserved-identifier
+  `, so that it does not warn
+  on macros starting with underscore and lowercase letter.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
@@ -102,13 +102,15 @@
 }
 
 static bool startsWithUnderscoreInGlobalNamespace(StringRef Name,
-  bool IsInGlobalNamespace) {
-  return IsInGlobalNamespace && Name.size() >= 1 && Name[0] == '_';
+  bool IsInGlobalNamespace,
+  bool IsMacro) {
+  return !IsMacro && IsInGlobalNamespace && Name.size() >= 1 && Name[0] == '_';
 }
 
 static std::optional
-getUnderscoreGlobalNamespaceFixup(StringRef Name, bool IsInGlobalNamespace) {
-  if (startsWithUnderscoreInGlobalNamespace(Name, IsInGlobalNamespace))
+getUnderscoreGlobalNamespaceFixup(StringRef Name, bool IsInGlobalNamespace,
+  bool IsMacro) {
+  if (startsWithUnderscoreInGlobalNamespace(Name, IsInGlobalNamespace, IsMacro))
 return std::string(Name.drop_front(1));
   return std::nullopt;
 }
@@ -123,7 +125,7 @@
 }
 
 static std::optional
-getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace,
+getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace, bool IsMacro,
const LangOptions , bool Invert,
ArrayRef AllowedIdentifiers) {
   assert(!Name.empty());
@@ -158,15 +160,16 @@
   AppendFailure(DoubleUnderscoreTag, std::move(*Fixup));
 if (auto Fixup = getUnderscoreCapitalFixup(InProgressFixup()))
   AppendFailure(UnderscoreCapitalTag, std::move(*Fixup));
-if (auto Fixup = getUnderscoreGlobalNamespaceFixup(InProgressFixup(),
-   IsInGlobalNamespace))
+if (auto Fixup = getUnderscoreGlobalNamespaceFixup(
+InProgressFixup(), IsInGlobalNamespace, IsMacro))
   AppendFailure(GlobalUnderscoreTag, std::move(*Fixup));
 
 return Info;
   }
   

[PATCH] D156608: [clang-tidy] Do not warn on macros starting with underscore and lowercase letter in bugprone-reserved-identifier

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Fixes #64130


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156608

Files:
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier.cpp
@@ -45,7 +45,7 @@
 
 #define __macro(m) int m = 0
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '__macro', which is a reserved identifier [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define macro(m) int m = 0{{$}}
+// CHECK-FIXES: {{^}}#define _macro(m) int m = 0{{$}}
 
 namespace __ns {
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration uses identifier '__ns', which is a reserved identifier [bugprone-reserved-identifier]
@@ -115,9 +115,9 @@
 
 //
 
+// OK, this rule does not apply to macros:
+// https://github.com/llvm/llvm-project/issues/64130#issuecomment-1655751676
 #define _macro(m) int m = 0
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_macro', which is reserved in the global namespace [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define macro(m) int m = 0{{$}}
 
 namespace _ns {
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration uses identifier '_ns', which is reserved in the global namespace [bugprone-reserved-identifier]
@@ -171,10 +171,9 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: declaration uses identifier '_', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]
 // CHECK-FIXES: {{^}}int _;{{$}}
 
+// This should not trigger a warning
 // https://github.com/llvm/llvm-project/issues/52895
 #define _5_kmph_rpm 459
-// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: declaration uses identifier '_5_kmph_rpm', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]
-// CHECK-FIXES: {{^}}#define _5_kmph_rpm 459{{$}}
 
 // these should pass
 #define MACRO(m) int m = 0
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -140,6 +140,10 @@
 Changes in existing checks
 ^^
 
+- Fixed bug in :doc:`bugprone-reserved-identifier
+  `, so that it does not warn
+  on macros starting with underscore and lowercase letter.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
@@ -102,13 +102,15 @@
 }
 
 static bool startsWithUnderscoreInGlobalNamespace(StringRef Name,
-  bool IsInGlobalNamespace) {
-  return IsInGlobalNamespace && Name.size() >= 1 && Name[0] == '_';
+  bool IsInGlobalNamespace,
+  bool IsMacro) {
+  return !IsMacro && IsInGlobalNamespace && Name.size() >= 1 && Name[0] == '_';
 }
 
 static std::optional
-getUnderscoreGlobalNamespaceFixup(StringRef Name, bool IsInGlobalNamespace) {
-  if (startsWithUnderscoreInGlobalNamespace(Name, IsInGlobalNamespace))
+getUnderscoreGlobalNamespaceFixup(StringRef Name, bool IsInGlobalNamespace,
+  bool IsMacro) {
+  if (startsWithUnderscoreInGlobalNamespace(Name, IsInGlobalNamespace, IsMacro))
 return std::string(Name.drop_front(1));
   return std::nullopt;
 }
@@ -123,7 +125,7 @@
 }
 
 static std::optional
-getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace,
+getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace, bool IsMacro,
const LangOptions , bool Invert,
ArrayRef AllowedIdentifiers) {
   assert(!Name.empty());
@@ -158,15 +160,17 @@
   AppendFailure(DoubleUnderscoreTag, std::move(*Fixup));
 if (auto Fixup = getUnderscoreCapitalFixup(InProgressFixup()))
   AppendFailure(UnderscoreCapitalTag, std::move(*Fixup));
-if (auto Fixup = getUnderscoreGlobalNamespaceFixup(InProgressFixup(),
-   IsInGlobalNamespace))
+if (auto Fixup = getUnderscoreGlobalNamespaceFixup(
+InProgressFixup(), 

[PATCH] D156452: [clang-tidy] Sort options in --dump-config

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, thanks for fixing!




Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:90
+std::vector> SortedOptions;
+SortedOptions.reserve(Options.size());
+for (auto  : Options) {

PiotrZSL wrote:
> carlosgalvezp wrote:
> > I believe you can pass this directly to the constructor in the previous 
> > line, to skip the `reserve()` part.
> No, constructor takes `resize` argument, not `reserve`.
You are right!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156452

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

[PATCH] D156452: [clang-tidy] Sort options in --dump-config

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Looks great, small comments!




Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:89
   if (IO.outputting()) {
+std::vector> SortedOptions;
+SortedOptions.reserve(Options.size());

Maybe add a one-line comment like `Ensure check options are sorted` to more 
quickly grasp what the block of code below does?



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:90
+std::vector> SortedOptions;
+SortedOptions.reserve(Options.size());
+for (auto  : Options) {

I believe you can pass this directly to the constructor in the previous line, 
to skip the `reserve()` part.



Comment at: clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp:58
+
+// RUN: clang-tidy --checks="-*,readability-identifier-*" --dump-config | grep 
readability-identifier-naming | sort -c

Use the long name of the option, i.e. `--check`, so it's easier to see what 
it's doing without having to check the manual.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156452

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

[PATCH] D156303: [clang-tidy] Remove AnalyzeTemporaryDestructors configuration option

[Carlos Galvez via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb7c6b39651b3: [clang-tidy] Remove 
AnalyzeTemporaryDestructors configuration option (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156303

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp


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
@@ -81,7 +81,6 @@
   "HeaderFileExtensions: [\"\",\"h\",\"hh\",\"hpp\",\"hxx\"]\n"
   "ImplementationFileExtensions: [\"c\",\"cc\",\"cpp\",\"cxx\"]\n"
   "HeaderFilterRegex: \".*\"\n"
-  "AnalyzeTemporaryDtors: true\n"
   "User: some.user",
   "Options"));
   EXPECT_TRUE(!!Options);
@@ -115,7 +114,6 @@
   HeaderFileExtensions: ["h","hh"]
   ImplementationFileExtensions: ["c","cc"]
   HeaderFilterRegex: "filter1"
-  AnalyzeTemporaryDtors: true
   User: user1
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
@@ -130,7 +128,6 @@
   HeaderFileExtensions: ["hpp","hxx"]
   ImplementationFileExtensions: ["cpp","cxx"]
   HeaderFilterRegex: "filter2"
-  AnalyzeTemporaryDtors: false
   User: user2
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -108,6 +108,9 @@
   functionality using the newly added command line option
   `--enable-module-headers-parsing`.
 
+- Remove configuration option `AnalyzeTemporaryDestructors`, which was 
deprecated since
+  :program:`clang-tidy` 16.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -157,14 +157,12 @@
 
 template <> struct MappingTraits {
   static void mapping(IO , ClangTidyOptions ) {
-bool Ignored = false;
 mapChecks(IO, Options.Checks);
 IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
 IO.mapOptional("HeaderFileExtensions", Options.HeaderFileExtensions);
 IO.mapOptional("ImplementationFileExtensions",
Options.ImplementationFileExtensions);
 IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
-IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // deprecated
 IO.mapOptional("FormatStyle", Options.FormatStyle);
 IO.mapOptional("User", Options.User);
 IO.mapOptional("CheckOptions", Options.CheckOptions);


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
@@ -81,7 +81,6 @@
   "HeaderFileExtensions: [\"\",\"h\",\"hh\",\"hpp\",\"hxx\"]\n"
   "ImplementationFileExtensions: [\"c\",\"cc\",\"cpp\",\"cxx\"]\n"
   "HeaderFilterRegex: \".*\"\n"
-  "AnalyzeTemporaryDtors: true\n"
   "User: some.user",
   "Options"));
   EXPECT_TRUE(!!Options);
@@ -115,7 +114,6 @@
   HeaderFileExtensions: ["h","hh"]
   ImplementationFileExtensions: ["c","cc"]
   HeaderFilterRegex: "filter1"
-  AnalyzeTemporaryDtors: true
   User: user1
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
@@ -130,7 +128,6 @@
   HeaderFileExtensions: ["hpp","hxx"]
   ImplementationFileExtensions: ["cpp","cxx"]
   HeaderFilterRegex: "filter2"
-  AnalyzeTemporaryDtors: false
   User: user2
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -108,6 +108,9 @@
   functionality using the newly added command line option
   `--enable-module-headers-parsing`.
 
+- Remove configuration option `AnalyzeTemporaryDestructors`, which was deprecated since
+  :program:`clang-tidy` 16.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- 

[PATCH] D156303: [clang-tidy] Remove AnalyzeTemporaryDestructors configuration option

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Since it was deprecated since clang-tidy 16.

Fixes #62020


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156303

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp


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
@@ -81,7 +81,6 @@
   "HeaderFileExtensions: [\"\",\"h\",\"hh\",\"hpp\",\"hxx\"]\n"
   "ImplementationFileExtensions: [\"c\",\"cc\",\"cpp\",\"cxx\"]\n"
   "HeaderFilterRegex: \".*\"\n"
-  "AnalyzeTemporaryDtors: true\n"
   "User: some.user",
   "Options"));
   EXPECT_TRUE(!!Options);
@@ -115,7 +114,6 @@
   HeaderFileExtensions: ["h","hh"]
   ImplementationFileExtensions: ["c","cc"]
   HeaderFilterRegex: "filter1"
-  AnalyzeTemporaryDtors: true
   User: user1
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
@@ -130,7 +128,6 @@
   HeaderFileExtensions: ["hpp","hxx"]
   ImplementationFileExtensions: ["cpp","cxx"]
   HeaderFilterRegex: "filter2"
-  AnalyzeTemporaryDtors: false
   User: user2
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -108,6 +108,9 @@
   functionality using the newly added command line option
   `--enable-module-headers-parsing`.
 
+- Remove configuration option `AnalyzeTemporaryDestructors`, which was 
deprecated since
+  :program:`clang-tidy` 16.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -157,14 +157,12 @@
 
 template <> struct MappingTraits {
   static void mapping(IO , ClangTidyOptions ) {
-bool Ignored = false;
 mapChecks(IO, Options.Checks);
 IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
 IO.mapOptional("HeaderFileExtensions", Options.HeaderFileExtensions);
 IO.mapOptional("ImplementationFileExtensions",
Options.ImplementationFileExtensions);
 IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
-IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // deprecated
 IO.mapOptional("FormatStyle", Options.FormatStyle);
 IO.mapOptional("User", Options.User);
 IO.mapOptional("CheckOptions", Options.CheckOptions);


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
@@ -81,7 +81,6 @@
   "HeaderFileExtensions: [\"\",\"h\",\"hh\",\"hpp\",\"hxx\"]\n"
   "ImplementationFileExtensions: [\"c\",\"cc\",\"cpp\",\"cxx\"]\n"
   "HeaderFilterRegex: \".*\"\n"
-  "AnalyzeTemporaryDtors: true\n"
   "User: some.user",
   "Options"));
   EXPECT_TRUE(!!Options);
@@ -115,7 +114,6 @@
   HeaderFileExtensions: ["h","hh"]
   ImplementationFileExtensions: ["c","cc"]
   HeaderFilterRegex: "filter1"
-  AnalyzeTemporaryDtors: true
   User: user1
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
@@ -130,7 +128,6 @@
   HeaderFileExtensions: ["hpp","hxx"]
   ImplementationFileExtensions: ["cpp","cxx"]
   HeaderFilterRegex: "filter2"
-  AnalyzeTemporaryDtors: false
   User: user2
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -108,6 +108,9 @@
   functionality using the newly added command line option
   `--enable-module-headers-parsing`.
 
+- Remove configuration option `AnalyzeTemporaryDestructors`, which was deprecated since
+  :program:`clang-tidy` 16.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- 

[PATCH] D156161: [clang-tidy] Add --enable-module-headers-parsing option

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM! Feel free to add the comment about the implications of using the flag in 
the docs.




Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:270
+for C++20 and above, empowering specific checks
+to detect macro definitions within modules.
+)"),

PiotrZSL wrote:
> carlosgalvezp wrote:
> > Should we document the implications/risks of enabling this, so people are 
> > informed? Also the fact that is experimental and subject to change.
> Something like "May cause performance & parsing issues, and therefore is 
> considered experimental." ? I'ts fine with me.
Sounds good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156161

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

[PATCH] D156161: [clang-tidy] Add --enable-module-headers-parsing option

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Do we want to keep the `experimental` word in the flag?




Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:266
 
+static cl::opt 
EnableModuleHeadersParsing("enable-module-headers-parsing",
+desc(R"(

--experimental-enable-module-headers-parsing



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:270
+for C++20 and above, empowering specific checks
+to detect macro definitions within modules.
+)"),

Should we document the implications/risks of enabling this, so people are 
informed? Also the fact that is experimental and subject to change.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:129
+  functionality using the newly added command line option
+  `--enable-module-headers-parsing`.
+

experimental


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156161

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

[PATCH] D156024: [clang-tidy] Add --experimental-disable-module-headers-parsing option

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:202
 
+  bool canEnableModuleHeadersParsing() const {
+return !DisableModuleHeadersParsing;

Add docs?



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:266
 
+static cl::opt
+DisableModuleHeadersParsing("experimental-disable-module-headers-parsing",

Add documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156024

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

[PATCH] D156161: [clang-tidy] Add --experimental-enable-module-headers-parsing option

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Please squash into previous patch, I see no reason to make them into separate 
commits. The first one is missing Release Notes, for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156161

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

[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75
+  Diags(new DiagnosticIDs,
+new DiagnosticOptions(Compiler.getDiagnosticOpts()),
 new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),

PiotrZSL wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > carlosgalvezp wrote:
> > > > PiotrZSL wrote:
> > > > > carlosgalvezp wrote:
> > > > > > PiotrZSL wrote:
> > > > > > > carlosgalvezp wrote:
> > > > > > > > When downloading your patch, this seems to not be needed to 
> > > > > > > > make the tests pass, should it be removed?
> > > > > > > No idea, it seem reasonable.
> > > > > > Do you mean it seems reasonable to keep it, or reasonable to remove 
> > > > > > it?
> > > > > reasonable to keep it, we want both DiagEngines to have same settings
> > > > Reason I ask is that it seems the majority of `DiagnosticOptions` are 
> > > > initialized with default ctor:
> > > > 
> > > > ```
> > > > $ git grep -E " DiagnosticOptions\(\w" | wc -l
> > > > 3
> > > > $ git grep -E " DiagnosticOptions\(\)" | wc -l
> > > > 74
> > > > ```
> > > > 
> > > > > we want both DiagEngines to have same settings
> > > > 
> > > > Do you know where "the other" `DiagEngine` is initialized? The one I 
> > > > can find is initialized without the compiler opts.
> > > > 
> > > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544
> > > > 
> > > > Since the added code does not have any impact on the result of the 
> > > > test, I'd argue that either the test is insufficient or the added code 
> > > > is dead code that should be removed.
> > > sure, maybe ProcessWarningOptions will override it anyway, I didnt check 
> > > more deeply.
> > In that case, could you please revert the change to this line, since it's 
> > not needed?
> Yes, but later today.
Ok! I will approve the patch now so you don't need to wait for me to land it 
before branching tomorrow


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056

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

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

The review is marked as accepted, should we land it? Let me know if you need 
help with that :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890

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

[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75
+  Diags(new DiagnosticIDs,
+new DiagnosticOptions(Compiler.getDiagnosticOpts()),
 new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),

PiotrZSL wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > carlosgalvezp wrote:
> > > > PiotrZSL wrote:
> > > > > carlosgalvezp wrote:
> > > > > > When downloading your patch, this seems to not be needed to make 
> > > > > > the tests pass, should it be removed?
> > > > > No idea, it seem reasonable.
> > > > Do you mean it seems reasonable to keep it, or reasonable to remove it?
> > > reasonable to keep it, we want both DiagEngines to have same settings
> > Reason I ask is that it seems the majority of `DiagnosticOptions` are 
> > initialized with default ctor:
> > 
> > ```
> > $ git grep -E " DiagnosticOptions\(\w" | wc -l
> > 3
> > $ git grep -E " DiagnosticOptions\(\)" | wc -l
> > 74
> > ```
> > 
> > > we want both DiagEngines to have same settings
> > 
> > Do you know where "the other" `DiagEngine` is initialized? The one I can 
> > find is initialized without the compiler opts.
> > 
> > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544
> > 
> > Since the added code does not have any impact on the result of the test, 
> > I'd argue that either the test is insufficient or the added code is dead 
> > code that should be removed.
> sure, maybe ProcessWarningOptions will override it anyway, I didnt check more 
> deeply.
In that case, could you please revert the change to this line, since it's not 
needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056

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

[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75
+  Diags(new DiagnosticIDs,
+new DiagnosticOptions(Compiler.getDiagnosticOpts()),
 new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),

PiotrZSL wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > carlosgalvezp wrote:
> > > > When downloading your patch, this seems to not be needed to make the 
> > > > tests pass, should it be removed?
> > > No idea, it seem reasonable.
> > Do you mean it seems reasonable to keep it, or reasonable to remove it?
> reasonable to keep it, we want both DiagEngines to have same settings
Reason I ask is that it seems the majority of `DiagnosticOptions` are 
initialized with default ctor:

```
$ git grep -E " DiagnosticOptions\(\w" | wc -l
3
$ git grep -E " DiagnosticOptions\(\)" | wc -l
74
```

> we want both DiagEngines to have same settings

Do you know where "the other" `DiagEngine` is initialized? The one I can find 
is initialized without the compiler opts.

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544

Since the added code does not have any impact on the result of the test, I'd 
argue that either the test is insufficient or the added code is dead code that 
should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056

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

[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75
+  Diags(new DiagnosticIDs,
+new DiagnosticOptions(Compiler.getDiagnosticOpts()),
 new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),

PiotrZSL wrote:
> carlosgalvezp wrote:
> > When downloading your patch, this seems to not be needed to make the tests 
> > pass, should it be removed?
> No idea, it seem reasonable.
Do you mean it seems reasonable to keep it, or reasonable to remove it?



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83
   Diags.setSourceManager();
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
 

PiotrZSL wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > carlosgalvezp wrote:
> > > > A bit unclear to me why we should add this line here, grepping for this 
> > > > function in the repo I only find hits in the `clang` folder. How come 
> > > > it's not needed in other places?
> > > We create here new Preprocessor (line 96) and new DiagEngine (line 74), 
> > > when C++20/Modules are enabled this class is register as an second 
> > > Preprocessor and both are (+-) executed.
> > > Unfortunately when we pass `-Wno-macro-redefined` it's pass only to 
> > > original DiagEngine, and we run into situation when warning is suppressed 
> > > by first DiagEngine, but not by second that is used by second 
> > > Preprocessor. 
> > > 
> > > Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, 
> > > as it's does not apply settings, only calling this function apply them. 
> > > (somehow).
> > > This is gray area for me.
> > > 
> > > More about problem here: 
> > > https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628
> > Thanks for the explanation! I'm not sure what the best way forward is. 
> > Would it make sense to add some `TODO` or `FIXME` comment to further 
> > investigate in the future if we want that line of code ?
> Yes, FIXME could be added, but ExpandModularHeadersPPCallbacks got also other 
> issues, for example with TargetTriple propagation and __has_builtin, looks 
> like all those got same source, simply we shoudn't create separate 
> Preprocessor.
Agreed, the whole thing looks fishy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056

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

[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

A thought came to mind - since we are doing workarounds anyway, would it be 
easier to ask people to simply add `-clang-diagnostic*` to the `Checks` in 
their config file? It's fair to assume they will get those warnings when 
compiling the code. I feel the more workarounds we add in the code the harder 
it will be to clean it up later :)




Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75
+  Diags(new DiagnosticIDs,
+new DiagnosticOptions(Compiler.getDiagnosticOpts()),
 new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),

When downloading your patch, this seems to not be needed to make the tests 
pass, should it be removed?



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83
   Diags.setSourceManager();
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
 

PiotrZSL wrote:
> carlosgalvezp wrote:
> > A bit unclear to me why we should add this line here, grepping for this 
> > function in the repo I only find hits in the `clang` folder. How come it's 
> > not needed in other places?
> We create here new Preprocessor (line 96) and new DiagEngine (line 74), when 
> C++20/Modules are enabled this class is register as an second Preprocessor 
> and both are (+-) executed.
> Unfortunately when we pass `-Wno-macro-redefined` it's pass only to original 
> DiagEngine, and we run into situation when warning is suppressed by first 
> DiagEngine, but not by second that is used by second Preprocessor. 
> 
> Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, as 
> it's does not apply settings, only calling this function apply them. 
> (somehow).
> This is gray area for me.
> 
> More about problem here: 
> https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628
Thanks for the explanation! I'm not sure what the best way forward is. Would it 
make sense to add some `TODO` or `FIXME` comment to further investigate in the 
future if we want that line of code ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056

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

[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83
   Diags.setSourceManager();
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
 

A bit unclear to me why we should add this line here, grepping for this 
function in the repo I only find hits in the `clang` folder. How come it's not 
needed in other places?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056

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

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I have opened a refactoring ticket here: 
https://github.com/llvm/llvm-project/issues/64037


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890

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

[PATCH] D156031: [clang-tidy] Ignore implcit casts in cppcoreguidelines-owning-memory

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156031

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

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }

gribozavr2 wrote:
> carlosgalvezp wrote:
> > If there's no need for `absl` here, why do we need to add `folly`?
> This is the branch for `RD.getName() == "Optional"` with a capital "O" 
> because `base::Optional`, `folly;:Optional` are spelled with a capital "O".
> 
> Abseil provides `absl::optional` and it is handled under `RD.getName() == 
> "optional"` together with `std::optional` above.
Got it, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890

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

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[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.

> I think it'd be good to add reviewers there

I realize the CodeOwners for Analysis are already in the list of reviewers, I 
won't interfere then :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890

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

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D155890#4523262 , @gribozavr2 
wrote:

> In D155890#4523243 , @adukeman 
> wrote:
>
>> In D155890#4522266 , @ymandel 
>> wrote:
>>
>>> In D155890#4521266 , 
>>> @carlosgalvezp wrote:
>>>
 This should be a configuration option, we should not hardcore 
 project-specific things in the source code.
>>>
>>> I agree, but we already are hardcoding specific types -- I think this is a 
>>> separate (and valid) critique of the design. I'd propose filing an issue on 
>>> the github tracker and we can follow up there.  I, for one, would love to 
>>> review such a change but don't have the time to write it.
>>
>> Is moving these values to config an appropriate task for somebody like me 
>> new to working on clang-tidy? I'd be happy to merge this and then try the 
>> transition to a config assuming there's some similar examples I can borrow 
>> from elsewhere in the codebase.
>
> I think it can be a good starter task for a new engineer on the project. 
> However, don't underestimate this problem, it will require the code to be 
> refactored a little bit. For example, the function `hasOptionalClassName` 
> needs restructuring so that it can accept class names from a list. Not a lot 
> of work, but it isn't mechanically replacing string literals with a variable 
> either.

Indeed this is not "the standard" CT check, the core is part of Clang so I 
think it'd be good to add reviewers there as well in case this affects other 
parts of the codebase. In that sense it does not seen as trivial as I thought 
to make this user configurable, so perhaps opening a ticket and solve it there 
is a faster way forward.




Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }

If there's no need for `absl` here, why do we need to add `folly`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890

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

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Carlos Galvez via Phabricator via cfe-commits]

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

This should be a configuration option, we should not hardcore project-specific 
things in the source code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb70e6e968192: [clang-tidy] Warn only for copyable/movable 
classes in cppcoreguidelines-avoid… (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = 

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

carlosgalvezp wrote:
> PiotrZSL wrote:
> > carlosgalvezp wrote:
> > > PiotrZSL wrote:
> > > > Check first type, should be cheaper and consider mering those two. 
> > > Thanks for the tip! I'm not familiar with having multiple binds in the 
> > > same `addMatcher` call. Do I still need to keep the `bind("ref")` at the 
> > > end?
> > No, bind at the end need to be removed (I forgot about that).
> Somehow I didn't manage to get it to work with your proposed change and 
> removing `bind`, I feel I'm walking on thin ice here :) 
Doing it from scratch helped, probably some parenthesis that was off!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

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

Merge matchers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Using `hasSimpleCopyConstructor` and so on greatly simplifies the logic, great! 
Let me know if you are happy with it or I should go ahead and merge.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

PiotrZSL wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > Check first type, should be cheaper and consider mering those two. 
> > Thanks for the tip! I'm not familiar with having multiple binds in the same 
> > `addMatcher` call. Do I still need to keep the `bind("ref")` at the end?
> No, bind at the end need to be removed (I forgot about that).
Somehow I didn't manage to get it to work with your proposed change and 
removing `bind`, I feel I'm walking on thin ice here :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541961.
carlosgalvezp marked 3 inline comments as done.
carlosgalvezp added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: 

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D155625#4512123 , @PiotrZSL wrote:

> LGTM, but I'm not sure if isCopyableOrMovable will always work correctly, for 
> a user defined special members it looks ok, but for some implicit ones I 
> worry it may not always work. Probably things like 
> "(hasSimpleCopyAssigment()) || (hasUserDeclaredCopyAssigment() && check here 
> if its not deleted)" would be needed.
> Thing is that CXXRecordDecl got most info (in confused way), there are things 
> like DefaultedMoveConstructorIsDeleted that could be used to verify somehow 
> base class.

Yeah I spent a lot of time trying to figure this out, somehow I expected this 
logic to already exist somewhere in Sema! Thanks for the pointers, I will 
experiment with those functions as well.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

PiotrZSL wrote:
> Check first type, should be cheaper and consider mering those two. 
Thanks for the tip! I'm not familiar with having multiple binds in the same 
`addMatcher` call. Do I still need to keep the `bind("ref")` at the end?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D147955: [clang-tidy] Extend CheckOptions to support grouping checks options

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp requested changes to this revision.
carlosgalvezp added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:94-95
+// Nested option names need to be converted to null terminated strings for
+// yaml serialization
+llvm::SmallString<64> Buffer;
+auto ToCString = [](StringRef Str) {

This function has become very large and hard to read. Would it be possible to 
split it into subfunctions so it's easier to follow? I think it would make 
sense to create 1 function per different format supported (I believe with this 
patch there's now 3 different formats supported).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147955

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

[PATCH] D153423: [clang-tidy] Allow explicit throwing in bugprone-exception-escape for special functions

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, thanks for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153423

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

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

Split getInfo function into 2 functions, remove
structured bindings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: 

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

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

Remove extra newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

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

Fix typo in release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,131 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

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

Remove confusing comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,131 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, shchenz, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Since that's what the guidelines require.

Fixes #63733


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.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/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
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,132 @@
 auto c5 = x5;
   };
 }
+
+// Classes without any copy or move operation are exempted from the rule
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  

[PATCH] D153458: [clang-tidy] Model noexcept more properly in bugprone-exception-escape

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153458

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

[PATCH] D151495: [clang-tidy] Improve build-in type handling in bugprone-swapped-arguments

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, thank you for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151495

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

[PATCH] D153458: [clang-tidy] Model noexcept more properly in bugprone-exception-escape

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Looks good, great to see all these issues fixed! Have a couple small comments.




Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:319
 
+static bool cannotThrow(const FunctionDecl *Func) {
+  const auto *FunProto = Func->getType()->getAs();

PiotrZSL wrote:
> isuckatcs wrote:
> > Put this in the anonymous namespace above please to remain consistent.
> well, llvm style require `static`, if we want to be consistent, I can change 
> all other into static later.
Nit: I personally find it more readable as "canThrow". People with issues 
reading double negations will probably appreciate that too :) But if you 
strongly prefer this name I won't oppose.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:330-332
+return NoexceptExpr && !NoexceptExpr->isValueDependent() &&
+   NoexceptExpr->EvaluateAsBooleanCondition(
+   Result, Func->getASTContext(), true) &&

Would be good to write a small comment documenting this logic.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:332
+   NoexceptExpr->EvaluateAsBooleanCondition(
+   Result, Func->getASTContext(), true) &&
+   Result;

Would be good to use the syntax `/* Parameter = */true` to document what this 
magic `true` means.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153458

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

[PATCH] D150071: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the review! Fixed your comments before landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150071

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

[PATCH] D150071: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
carlosgalvezp marked 2 inline comments as done.
Closed by commit rG0d6d8a853a6e: [clang-tidy] Fix bugprone-assert-side-effect 
to actually give warnings (authored by carlosgalvezp).

Changed prior to commit:
  https://reviews.llvm.org/D150071?vs=520194=520732#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150071

Files:
  clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
@@ -1,47 +1,5 @@
-// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'MyClass::badButIgnoredFunc'}]}" -- -fexceptions
-
-//===--- assert definition block --===//
-int abort() { return 0; }
-
-#ifdef NDEBUG
-#define assert(x) 1
-#else
-#define assert(x)  \
-  if (!(x))\
-  (void)abort()
-#endif
-
-void print(...);
-#define assert2(e) (__builtin_expect(!(e), 0) ?\
-   print (#e, __FILE__, __LINE__) : (void)0)
-
-#ifdef NDEBUG
-#define my_assert(x) 1
-#else
-#define my_assert(x)   \
-  ((void)((x) ? 1 : abort()))
-#endif
-
-#ifdef NDEBUG
-#define not_my_assert(x) 1
-#else
-#define not_my_assert(x)   \
-  if (!(x))\
-  (void)abort()
-#endif
-
-#define real_assert(x) ((void)((x) ? 1 : abort()))
-#define wrap1(x) real_assert(x)
-#define wrap2(x) wrap1(x)
-#define convoluted_assert(x) wrap2(x)
-
-#define msvc_assert(expression) (void)(\
-(!!(expression)) ||\
-(abort(), 0)   \
-)
-
-
-//===--===//
+// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'MyClass::badButIgnoredFunc'}]}" -- -fexceptions -I %S/Inputs/assert-side-effect
+#include 
 
 bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
 
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
@@ -0,0 +1,40 @@
+#pragma clang system_header
+
+int abort();
+
+#ifdef NDEBUG
+#define assert(x) 1
+#else
+#define assert(x)  \
+  if (!(x))\
+  (void)abort()
+#endif
+
+void print(...);
+#define assert2(e) (__builtin_expect(!(e), 0) ?\
+   print (#e, __FILE__, __LINE__) : (void)0)
+
+#ifdef NDEBUG
+#define my_assert(x) 1
+#else
+#define my_assert(x)   \
+  ((void)((x) ? 1 : abort()))
+#endif
+
+#ifdef NDEBUG
+#define not_my_assert(x) 1
+#else
+#define not_my_assert(x)   \
+  if (!(x))\
+  (void)abort()
+#endif
+
+#define real_assert(x) ((void)((x) ? 1 : abort()))
+#define wrap1(x) real_assert(x)
+#define wrap2(x) wrap1(x)
+#define convoluted_assert(x) wrap2(x)
+
+#define msvc_assert(expression) (void)(\
+(!!(expression)) ||\
+(abort(), 0)   \
+)
Index: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp

[PATCH] D148995: [clang-tidy] Extract areStatementsIdentical

[Carlos Galvez via Phabricator via cfe-commits]

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

> Separating the changes into two patches would only prolong this process and 
> potentially delay the completion of this task.

I understand the concern, but in practice it's actually the other way around. 2 
small and easy to review patches are more likely to get reviewed quickly than 
the same patches combined into 1 big patch. The easier you make life for 
reviewers, the more likely they are to be willing / have time to review a 
patch. When I post patches, I often try and reflect about what else I can 
remove or make more obvious/readable to make reviewer's life easier and improve 
my chances of getting a quick review. I understand it's more work on the author 
side, but from experience I've found there's a net gain to doing it.

Anyway, the patch is indeed fairly easy to review so I'll just approve it, 
thank you for the contribution!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148995

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

[PATCH] D150071: [clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Some time ago a patch was merged to disable all clang-tidy warnings
from system macros. This led to bugprone-assert-side-effect
silently no longer working, since the warnings came from a system
macro. The problem was not detected because the fake assert functions
were implemented in the same file as the test, instead of being a
system include like it's done in the real world.

Move the assert to a proper system header, and fix the code to
warn at the correct location.

This patch is breakdown from https://reviews.llvm.org/D147081
by PiotrZSL.

Fixes https://github.com/llvm/llvm-project/issues/62314


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150071

Files:
  clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
@@ -1,47 +1,5 @@
-// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'MyClass::badButIgnoredFunc'}]}" -- -fexceptions
-
-//===--- assert definition block --===//
-int abort() { return 0; }
-
-#ifdef NDEBUG
-#define assert(x) 1
-#else
-#define assert(x)  \
-  if (!(x))\
-  (void)abort()
-#endif
-
-void print(...);
-#define assert2(e) (__builtin_expect(!(e), 0) ?\
-   print (#e, __FILE__, __LINE__) : (void)0)
-
-#ifdef NDEBUG
-#define my_assert(x) 1
-#else
-#define my_assert(x)   \
-  ((void)((x) ? 1 : abort()))
-#endif
-
-#ifdef NDEBUG
-#define not_my_assert(x) 1
-#else
-#define not_my_assert(x)   \
-  if (!(x))\
-  (void)abort()
-#endif
-
-#define real_assert(x) ((void)((x) ? 1 : abort()))
-#define wrap1(x) real_assert(x)
-#define wrap2(x) wrap1(x)
-#define convoluted_assert(x) wrap2(x)
-
-#define msvc_assert(expression) (void)(\
-(!!(expression)) ||\
-(abort(), 0)   \
-)
-
-
-//===--===//
+// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'MyClass::badButIgnoredFunc'}]}" -- -fexceptions -isystem %S/Inputs/assert-side-effect
+#include 
 
 bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
 
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
@@ -0,0 +1,38 @@
+int abort() { return 0; }
+
+#ifdef NDEBUG
+#define assert(x) 1
+#else
+#define assert(x)  \
+  if (!(x))\
+  (void)abort()
+#endif
+
+void print(...);
+#define assert2(e) (__builtin_expect(!(e), 0) ?\
+   print (#e, __FILE__, __LINE__) : (void)0)
+
+#ifdef NDEBUG
+#define my_assert(x) 1
+#else
+#define my_assert(x)   \
+  ((void)((x) ? 1 : abort()))
+#endif
+
+#ifdef NDEBUG
+#define not_my_assert(x) 1
+#else
+#define not_my_assert(x)   \
+  if (!(x))\
+  (void)abort()
+#endif
+
+#define real_assert(x) ((void)((x) ? 1 : abort()))

[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM!

> A lot of our test files uses macros to differentiate between specific C++ 
> standards, why not do that here too?

It's not that easy, because the #ifdef lines do not remove the `// CHECK` 
comments, so to work around that it gets messy very easily. Splitting into 2 
files seems to be common practice in other tests for easier maintenance and 
readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148458

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

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG26f476286fbc: [clang-tidy] Support SystemHeaders in 
.clang-tidy (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149899

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  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/infrastructure/Inputs/system-headers/system_header.h
  clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

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
@@ -120,6 +120,7 @@
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
   UseColor: false
+  SystemHeaders: false
   )",
"Options1"));
   ASSERT_TRUE(!!Options1);
@@ -134,6 +135,7 @@
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
   UseColor: true
+  SystemHeaders: true
   )",
"Options2"));
   ASSERT_TRUE(!!Options2);
@@ -154,6 +156,9 @@
Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.has_value());
   EXPECT_TRUE(*Options.UseColor);
+
+  ASSERT_TRUE(Options.SystemHeaders.has_value());
+  EXPECT_TRUE(*Options.SystemHeaders);
 }
 
 namespace {
@@ -249,6 +254,17 @@
  DiagKind(llvm::SourceMgr::DK_Error),
  DiagPos(Options.range().Begin),
  DiagRange(Options.range();
+
+  Options = llvm::Annotations(R"(
+SystemHeaders: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+  testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+ DiagKind(llvm::SourceMgr::DK_Error),
+ DiagPos(Options.range().Begin),
+ DiagRange(Options.range();
 }
 
 namespace {
Index: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
@@ -0,0 +1,24 @@
+// RUN: clang-tidy -dump-config -system-headers=true | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -dump-config -system-headers=false | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include 
+// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+// CHECK-NO-SYSTEM-HEADERS-NOT: system_header.h:1:13: warning: single-argument constructors must be marked explicit

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

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

Revert unwanted change to UseColor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149899

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  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/infrastructure/Inputs/system-headers/system_header.h
  clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

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
@@ -120,6 +120,7 @@
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
   UseColor: false
+  SystemHeaders: false
   )",
"Options1"));
   ASSERT_TRUE(!!Options1);
@@ -134,6 +135,7 @@
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
   UseColor: true
+  SystemHeaders: true
   )",
"Options2"));
   ASSERT_TRUE(!!Options2);
@@ -154,6 +156,9 @@
Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.has_value());
   EXPECT_TRUE(*Options.UseColor);
+
+  ASSERT_TRUE(Options.SystemHeaders.has_value());
+  EXPECT_TRUE(*Options.SystemHeaders);
 }
 
 namespace {
@@ -249,6 +254,17 @@
  DiagKind(llvm::SourceMgr::DK_Error),
  DiagPos(Options.range().Begin),
  DiagRange(Options.range();
+
+  Options = llvm::Annotations(R"(
+SystemHeaders: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+  testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+ DiagKind(llvm::SourceMgr::DK_Error),
+ DiagPos(Options.range().Begin),
+ DiagRange(Options.range();
 }
 
 namespace {
Index: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
@@ -0,0 +1,24 @@
+// RUN: clang-tidy -dump-config -system-headers=true | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -dump-config -system-headers=false | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include 
+// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+// CHECK-NO-SYSTEM-HEADERS-NOT: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+
+// CHECK-CONFIG-NO-SYSTEM-HEADERS: SystemHeaders: false
+// 

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 520151.
carlosgalvezp added a comment.
Herald added a subscriber: arphaman.

- Add test where both command line and config settings are used.
- Document that the command-line option overrides the config option, for 
consistency with other command-line options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149899

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  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/infrastructure/Inputs/system-headers/system_header.h
  clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

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
@@ -120,6 +120,7 @@
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
   UseColor: false
+  SystemHeaders: false
   )",
"Options1"));
   ASSERT_TRUE(!!Options1);
@@ -134,6 +135,7 @@
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
   UseColor: true
+  SystemHeaders: true
   )",
"Options2"));
   ASSERT_TRUE(!!Options2);
@@ -154,6 +156,9 @@
Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.has_value());
   EXPECT_TRUE(*Options.UseColor);
+
+  ASSERT_TRUE(Options.SystemHeaders.has_value());
+  EXPECT_TRUE(*Options.SystemHeaders);
 }
 
 namespace {
@@ -249,6 +254,17 @@
  DiagKind(llvm::SourceMgr::DK_Error),
  DiagPos(Options.range().Begin),
  DiagRange(Options.range();
+
+  Options = llvm::Annotations(R"(
+SystemHeaders: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+  testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+ DiagKind(llvm::SourceMgr::DK_Error),
+ DiagPos(Options.range().Begin),
+ DiagRange(Options.range();
 }
 
 namespace {
Index: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
@@ -0,0 +1,24 @@
+// RUN: clang-tidy -dump-config -system-headers=true | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -dump-config -system-headers=false | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include 
+// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked 

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp:11
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' 
-header-filter='.*' -config='SystemHeaders: false' %s -- -isystem 
%S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS 
%s
+
+#include 

PiotrZSL wrote:
> I missing 2 types of tests here:
> - config value override, `-config='SystemHeaders: true' -system-headers=false`
> - config file override, when one file has `false`, other `true`, but 
> InheritConfig is used.
> Both should work, but would be good to test them.
Thanks for the input! Essentially I followed the same testing pattern as 
`UseColor`, which is also a global boolean option. 

Adding the first suggestion should be trivial in the current test case, but I 
have some doubts about the proposed second test. When I look at the 
`tests/clang-tidy/infrastructure/config-files.cpp` it only takes into account 
enabled checks, no other global options are tested such that one overrides the 
other. Therefore I wonder if there is a rationale for not having such level of 
testing, perhaps easier maintenance, given that it's already tested by the unit 
tests? In that case I would propose to not add such a test here, for 
consistency. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149899

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

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 519633.
carlosgalvezp edited the summary of this revision.
carlosgalvezp added a comment.

Update commit message with Github issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149899

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h
  clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

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
@@ -120,6 +120,7 @@
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
   UseColor: false
+  SystemHeaders: false
   )",
"Options1"));
   ASSERT_TRUE(!!Options1);
@@ -134,6 +135,7 @@
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
   UseColor: true
+  SystemHeaders: true
   )",
"Options2"));
   ASSERT_TRUE(!!Options2);
@@ -154,6 +156,9 @@
Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.has_value());
   EXPECT_TRUE(*Options.UseColor);
+
+  ASSERT_TRUE(Options.SystemHeaders.has_value());
+  EXPECT_TRUE(*Options.SystemHeaders);
 }
 
 namespace {
@@ -249,6 +254,17 @@
  DiagKind(llvm::SourceMgr::DK_Error),
  DiagPos(Options.range().Begin),
  DiagRange(Options.range();
+
+  Options = llvm::Annotations(R"(
+SystemHeaders: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+  testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+ DiagKind(llvm::SourceMgr::DK_Error),
+ DiagPos(Options.range().Begin),
+ DiagRange(Options.range();
 }
 
 namespace {
Index: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
@@ -0,0 +1,18 @@
+// RUN: clang-tidy -dump-config -system-headers=true | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -dump-config -system-headers=false | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include 
+// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+// CHECK-NO-SYSTEM-HEADERS-NOT: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+
+// CHECK-CONFIG-NO-SYSTEM-HEADERS: SystemHeaders: false
+// CHECK-CONFIG-SYSTEM-HEADERS: SystemHeaders: true
+// CHECK-OPT-PRESENT: --system-headers
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h
@@ -0,0 +1 @@
+class Foo { Foo(int); };
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -103,6 +103,9 @@
 
 - 

[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

A previous patch update the clang-tidy documentation
incorrectly claiming that SystemHeaders can be provided
in the .clang-tidy configuration file.

This patch adds support for it, together with tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149899

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h
  clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

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
@@ -120,6 +120,7 @@
   ExtraArgs: ['arg1', 'arg2']
   ExtraArgsBefore: ['arg-before1', 'arg-before2']
   UseColor: false
+  SystemHeaders: false
   )",
"Options1"));
   ASSERT_TRUE(!!Options1);
@@ -134,6 +135,7 @@
   ExtraArgs: ['arg3', 'arg4']
   ExtraArgsBefore: ['arg-before3', 'arg-before4']
   UseColor: true
+  SystemHeaders: true
   )",
"Options2"));
   ASSERT_TRUE(!!Options2);
@@ -154,6 +156,9 @@
Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.has_value());
   EXPECT_TRUE(*Options.UseColor);
+
+  ASSERT_TRUE(Options.SystemHeaders.has_value());
+  EXPECT_TRUE(*Options.SystemHeaders);
 }
 
 namespace {
@@ -249,6 +254,17 @@
  DiagKind(llvm::SourceMgr::DK_Error),
  DiagPos(Options.range().Begin),
  DiagRange(Options.range();
+
+  Options = llvm::Annotations(R"(
+SystemHeaders: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+  testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+ DiagKind(llvm::SourceMgr::DK_Error),
+ DiagPos(Options.range().Begin),
+ DiagRange(Options.range();
 }
 
 namespace {
Index: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
@@ -0,0 +1,18 @@
+// RUN: clang-tidy -dump-config -system-headers=true | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -dump-config -system-headers=false | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include 
+// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+// CHECK-NO-SYSTEM-HEADERS-NOT: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+
+// CHECK-CONFIG-NO-SYSTEM-HEADERS: SystemHeaders: false
+// CHECK-CONFIG-SYSTEM-HEADERS: SystemHeaders: true
+// CHECK-OPT-PRESENT: --system-headers
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h
@@ -0,0 +1 @@
+class Foo { 

[PATCH] D131319: [clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h:61
+private:
+  mutable std::optional HasIsPresent;
 };

njames93 wrote:
> PiotrZSL wrote:
> > no need for mutable, its used only form Check that isn't const.
> > Maybe better HasIsPresentCache ?
> Good spot.
Comment not yet addressed, do you intend to?



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:323
+  use the ``*and_present`` and ``*if_present`` templates added in 
+  `D123901 `_.
+

Would it make more sense to point to an actual commit in the repo? A review may 
or may not have been merged.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp:1
-// RUN: %check_clang_tidy %s llvm-prefer-isa-or-dyn-cast-in-conditionals %t
-
+// RDUN: %check_clang_tidy -check-suffixes=,NO-PRESENT %s 
llvm-prefer-isa-or-dyn-cast-in-conditionals %t
+// RUN: %check_clang_tidy -check-suffixes=,PRESENT %s 
llvm-prefer-isa-or-dyn-cast-in-conditionals %t -- -- -DISA_AND_PRESENT

Typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131319

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

[PATCH] D148995: [clang-tidy] Extract areStatementsIdentical

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the refactoring, great to remove some code duplication!

As a reviewer it's not possible for me to confidently review the functional 
changes to `areStatementsIdentical`, since they don't show side by side as they 
are in different files. Please perform the move in one NFC patch, and apply the 
functional changes in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148995

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

[PATCH] D148110: [clang-tidy] Ctor arguments are sequenced if ctor call is written as list-initialization.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

The commit message doesn't really tell me "what" this commit is fixing, it only 
points to a section of the Standard. It talks about "a false positive" but it 
doesn't tell what this FP is about. Could you write a little bit more about 
what the problem is? Preferably if you can link a Github issue describing the 
problem. The subject of the commit message should indicate which particular 
check it relates to.

Document change in release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148110

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

[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I would agree that the fact that a function throws or not can only be found out 
when analyzing the function definition (it's impossible to know from the 
declaration). There's also a duplicate diagnostic that I don't see much value 
on, so I would agree with this patch unless I'm missing some concrete use case 
I'm not aware of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148462

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

[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp:4
+void throwing_throw_nothing() throw() {
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in 
function 'throwing_throw_nothing' which should not throw exceptions
+  throw 1;

Please align comments



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp:11
+void implicit_int_thrower() {
+throw 5;
+}

Please use 2 chars indentation. This can be configured in the IDE.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:267
 const derived *p = 
-throw p; 
   } catch(base *) {

PiotrZSL wrote:
> carlosgalvezp wrote:
> > I run into this often as well. If you don't want to get push back during 
> > review because of this I advice you to disable the automatic trailing 
> > whitespace removal for this project. Regular source code will be fixed via 
> > clang-format anyway. Alternatively you will be asked to fix it in (another) 
> > NFC patch :) 
> > 
> > I personally don't mind a couple or two such fixes, but here there's a lot 
> > of them and really create noise, distracting from the actual patch.
> Excuses...
Excuses for what?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:555-557
+void explicit_int_thrower() noexcept(false) {
+  throw 1;
+}

PiotrZSL wrote:
> carlosgalvezp wrote:
> > Why this change?
> this test file tests only noexcept, not throw.
> i added throw 1 just so check would still see as an throw of integer type.
Ack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148458

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

[PATCH] D148793: [WIP][clang-tidy] Implement an include-cleaner check.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

I believe this should be discussed in an RFC. We already have the standalone 
`include-cleaner` tool, why is that not sufficient? Can it be extended instead? 
There's also the include-what-you-use 
 tool out there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793

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

[PATCH] D148697: [clang-tidy] Handle more cases of functions which should always be noexcept

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-special-functions.rst:10
+`performance-noexcept-special-functions 
<../performance/noexcept-special-functions.html>`_
+for more information.

PiotrZSL wrote:
> Add link to implemented cpp core guidelines rules.
> Add reference 
> C.66: Make move operations noexcept
> C.83: For value-like types, consider providing a noexcept swap function.
> Same goes to operator == (C.86 rule)
> And hash C.89 rule
> 
Since there are different rules for different functions it's probably easier to 
maintain if we have 1 check per rule. Having a large check doing multiple 
things and multiple reasons for change can lead to more bugs, churn, renames, 
etc. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148697

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

[PATCH] D148697: [clang-tidy] Handle more cases of functions which should always be noexcept

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

First of all thank you for the contribution! I had just a quick look so here's 
some very preliminary comments, will have more time to deeply review during the 
weekend:

- Unfortunately we cannot simply rename a check like this, since it breaks 
backwards compatibility. We need a period of 2 major releases where both the 
new check and the old check (marked as deprecated) co-exist, so people have 
enough time to port over. Only after those 2 releases can the old check be 
fully removed.
- Contrary to my comment on Github, I no longer think this should be a 
cppcoreguideline check, as it does **not** implement rule F.6. The enforcement 
required is to flag **all** functions (not just the special ones) that cannot 
throw and mark them as noexcept. That's a huge difference with a larger scope 
and impact in a codebase. I personally think that last bit is impractical in a 
real-world project, it would add too much noise.
- This check is also used as an alias to another guideline - HiCPP Rule 12.5.4, 
which strictly only covers move operators. Guideline checks  (HiCPP, 
CppCoreGuidelines) are stricter than the regular checks, and should only cover 
what they intend to cover - no more, no less.
- Special member functions are also copy constructors & assignments, yet those 
don't seem to be part of the rule?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148697

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

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb507bda45523: [clang-tidy] Add alias 
cppcoreguidelines-use-default-member-init (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148460

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst

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
@@ -477,6 +477,7 @@
`cppcoreguidelines-explicit-virtual-functions `_, `modernize-use-override `_, "Yes"
`cppcoreguidelines-macro-to-enum `_, `modernize-macro-to-enum `_, "Yes"
`cppcoreguidelines-non-private-member-variables-in-classes `_, `misc-non-private-member-variables-in-classes `_,
+   `cppcoreguidelines-use-default-member-init `_, `modernize-use-default-member-init `_,
`fuchsia-header-anon-namespaces `_, `google-build-namespaces `_,
`google-readability-braces-around-statements `_, `readability-braces-around-statements `_, "Yes"
`google-readability-function-size `_, `readability-function-size `_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-use-default-member-init
+.. meta::
+   :http-equiv=refresh: 5;URL=../modernize/use-default-member-init.html
+
+cppcoreguidelines-use-default-member-init
+=
+
+This check implements `C.48 `_ from the CppCoreGuidelines.
+
+The cppcoreguidelines-use-default-member-init check is an alias, please see
+`modernize-use-default-member-init <../modernize/use-default-member-init.html>`_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
@@ -23,6 +23,13 @@
 initializations already implemented as member initializers. For that purpose
 see check `modernize-use-default-member-init <../modernize/use-default-member-init.html>`_.
 
+.. note::
+
+  Enforcement of rule C.48 in this check is deprecated, to be removed in
+  :program:`clang-tidy` version 19 (only C.49 will be enforced by this check then).
+  Please use `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+  to enfoce rule C.48.
+
 Example 1
 -
 
@@ -85,6 +92,11 @@
 
 .. option:: UseAssignment
 
+   Note: this option is deprecated, to be removed in :program:`clang-tidy`
+   version 19. Please use the `UseAssignment` option from
+   `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+   instead.
+
If this option is set to `true` (by default `UseAssignment` from
`modernize-use-default-member-init
<../modernize/use-default-member-init.html>`_ will be used),
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -174,6 +174,11 @@
   ` to :doc:`bugprone-unsafe-functions
   ` was added.
 
+- New alias :doc:`cppcoreguidelines-use-default-member-init
+  ` to
+  :doc:`modernize-use-default-member-init
+  ` was added.
+
 Changes in existing checks
 ^^
 - Improved :doc:`readability-redundant-string-cstr
@@ -215,6 +220,11 @@
 - Deprecated :doc:`cert-dcl21-cpp
   ` check.
 
+- Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
+  `. Please use
+  :doc:`cppcoreguidelines-use-default-member-init
+  ` instead.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   ` check.
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ 

[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst:103
+{
+try
+{

Use 2 spaces indentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144748

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

[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:100
+void EmptyCatchCheck::check(const MatchFinder::MatchResult ) {
+  const auto *MatchedCatchStmt = Result.Nodes.getNodeAs("catch");
+

Assert that it's not null, or exit early.



Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:104
+  MatchedCatchStmt->getCatchLoc(),
+  "empty catch statements hide issues, to handle exceptions appropriately, 
"
+  "consider re-throwing, handling, or avoiding catch altogether");

Nit: I believe a semicolon or dot is more appropriate here



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst:147-153
+.. option:: IgnoreCatchWithKeywords
+
+This option can be used to ignore specific catch statements containing
+certain keywords. If a ``catch`` statement body contains (case-insensitive)
+any of the keywords listed in this semicolon-separated option, then the
+catch will be ignored, and no warning will be raised.
+Default value: `@TODO;@FIXME`.

I'm not sure this option is worth the added complexity - if people want to 
allow an empty catch with a comment, they might as well just add a `NOLINT` to 
suppress the warning? It will be even more clear that it's actually a problem: 
it's not a regular `TODO` that doesn't necessarily imply a problem, but rather 
something that a static analyzer considers a problem. 





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp:11
+{
+try
+{

Fix indentation to 2 chars.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp:16
+catch(const Exception&)
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty catch statements hide 
issues, to handle exceptions appropriately, consider re-throwing, handling, or 
avoiding catch altogether [bugprone-empty-catch]
+{

Align comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144748

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

[PATCH] D148444: [clang-tidy] Prevent `llvmlibc-inline-function-decl` triggering on lambdas

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvmlibc/InlineFunctionDeclCheck.cpp:43
+  // Consider only functions with an external and visible declaration.
+  if (const auto *MethodDecl = dyn_cast(FuncDecl))
+if (MethodDecl->getParent()->isLambda())

Since there are 2 lines under this `if`, please add braces for better 
readability.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvmlibc/inline-function-decl.hpp:64-67
+// CHECK-MESSAGES-NOT: :[[@LINE+4]]:3: warning: '__invoke' must be tagged with 
the LIBC_INLINE macro; the macro should be placed at the beginning of the 
declaration [llvmlibc-inline-function-decl]
+// CHECK-MESSAGES-NOT: :[[@LINE+3]]:3: warning: 'operator void (*)()' must be 
tagged with the LIBC_INLINE macro; the macro should be placed at the beginning 
of the declaration [llvmlibc-inline-function-decl]
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:3: warning: '~(lambda at [[FILENAME:.+]])' 
must be tagged with the LIBC_INLINE macro; the macro should be placed at the 
beginning of the declaration [llvmlibc-inline-function-decl]
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:6: warning: 'operator()' must be tagged 
with the LIBC_INLINE macro; the macro should be placed at the beginning of the 
declaration [llvmlibc-inline-function-decl]

Not needed, simply write a plain comment explaining why the check should not 
warn here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148444

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

[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:267
 const derived *p = 
-throw p; 
   } catch(base *) {

I run into this often as well. If you don't want to get push back during review 
because of this I advice you to disable the automatic trailing whitespace 
removal for this project. Regular source code will be fixed via clang-format 
anyway. Alternatively you will be asked to fix it in (another) NFC patch :) 

I personally don't mind a couple or two such fixes, but here there's a lot of 
them and really create noise, distracting from the actual patch.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:555-557
+void explicit_int_thrower() noexcept(false) {
+  throw 1;
+}

Why this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148458

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

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Carlos Galvez via Phabricator via cfe-commits]

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

Remove check from list of checks, since we do not add 
aliases there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148460

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst

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
@@ -477,6 +477,7 @@
`cppcoreguidelines-explicit-virtual-functions `_, `modernize-use-override `_, "Yes"
`cppcoreguidelines-macro-to-enum `_, `modernize-macro-to-enum `_, "Yes"
`cppcoreguidelines-non-private-member-variables-in-classes `_, `misc-non-private-member-variables-in-classes `_,
+   `cppcoreguidelines-use-default-member-init `_, `modernize-use-default-member-init `_,
`fuchsia-header-anon-namespaces `_, `google-build-namespaces `_,
`google-readability-braces-around-statements `_, `readability-braces-around-statements `_, "Yes"
`google-readability-function-size `_, `readability-function-size `_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-use-default-member-init
+.. meta::
+   :http-equiv=refresh: 5;URL=../modernize/use-default-member-init.html
+
+cppcoreguidelines-use-default-member-init
+=
+
+This check implements `C.48 `_ from the CppCoreGuidelines.
+
+The cppcoreguidelines-use-default-member-init check is an alias, please see
+`modernize-use-default-member-init <../modernize/use-default-member-init.html>`_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
@@ -23,6 +23,13 @@
 initializations already implemented as member initializers. For that purpose
 see check `modernize-use-default-member-init <../modernize/use-default-member-init.html>`_.
 
+.. note::
+
+  Enforcement of rule C.48 in this check is deprecated, to be removed in
+  :program:`clang-tidy` version 19 (only C.49 will be enforced by this check then).
+  Please use `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+  to enfoce rule C.48.
+
 Example 1
 -
 
@@ -85,6 +92,11 @@
 
 .. option:: UseAssignment
 
+   Note: this option is deprecated, to be removed in :program:`clang-tidy`
+   version 19. Please use the `UseAssignment` option from
+   `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+   instead.
+
If this option is set to `true` (by default `UseAssignment` from
`modernize-use-default-member-init
<../modernize/use-default-member-init.html>`_ will be used),
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -174,6 +174,11 @@
   ` to :doc:`bugprone-unsafe-functions
   ` was added.
 
+- New alias :doc:`cppcoreguidelines-use-default-member-init
+  ` to
+  :doc:`modernize-use-default-member-init
+  ` was added.
+
 Changes in existing checks
 ^^
 - Improved :doc:`readability-redundant-string-cstr
@@ -215,6 +220,11 @@
 - Deprecated :doc:`cert-dcl21-cpp
   ` check.
 
+- Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
+  `. Please use
+  :doc:`cppcoreguidelines-use-default-member-init
+  ` instead.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   ` check.
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ 

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Carlos Galvez via Phabricator via cfe-commits]

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

Add check to list of aliases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148460

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst

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
@@ -207,6 +207,7 @@
`cppcoreguidelines-rvalue-reference-param-not-moved `_,
`cppcoreguidelines-slicing `_,
`cppcoreguidelines-special-member-functions `_,
+   `cppcoreguidelines-use-default-member-init `_, "Yes"
`cppcoreguidelines-virtual-class-destructor `_, "Yes"
`darwin-avoid-spinlock `_,
`darwin-dispatch-once-nonstatic `_, "Yes"
@@ -477,6 +478,7 @@
`cppcoreguidelines-explicit-virtual-functions `_, `modernize-use-override `_, "Yes"
`cppcoreguidelines-macro-to-enum `_, `modernize-macro-to-enum `_, "Yes"
`cppcoreguidelines-non-private-member-variables-in-classes `_, `misc-non-private-member-variables-in-classes `_,
+   `cppcoreguidelines-use-default-member-init `_, `modernize-use-default-member-init `_,
`fuchsia-header-anon-namespaces `_, `google-build-namespaces `_,
`google-readability-braces-around-statements `_, `readability-braces-around-statements `_, "Yes"
`google-readability-function-size `_, `readability-function-size `_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-use-default-member-init
+.. meta::
+   :http-equiv=refresh: 5;URL=../modernize/use-default-member-init.html
+
+cppcoreguidelines-use-default-member-init
+=
+
+This check implements `C.48 `_ from the CppCoreGuidelines.
+
+The cppcoreguidelines-use-default-member-init check is an alias, please see
+`modernize-use-default-member-init <../modernize/use-default-member-init.html>`_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
@@ -23,6 +23,13 @@
 initializations already implemented as member initializers. For that purpose
 see check `modernize-use-default-member-init <../modernize/use-default-member-init.html>`_.
 
+.. note::
+
+  Enforcement of rule C.48 in this check is deprecated, to be removed in
+  :program:`clang-tidy` version 19 (only C.49 will be enforced by this check then).
+  Please use `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+  to enfoce rule C.48.
+
 Example 1
 -
 
@@ -85,6 +92,11 @@
 
 .. option:: UseAssignment
 
+   Note: this option is deprecated, to be removed in :program:`clang-tidy`
+   version 19. Please use the `UseAssignment` option from
+   `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+   instead.
+
If this option is set to `true` (by default `UseAssignment` from
`modernize-use-default-member-init
<../modernize/use-default-member-init.html>`_ will be used),
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -174,6 +174,11 @@
   ` to :doc:`bugprone-unsafe-functions
   ` was added.
 
+- New alias :doc:`cppcoreguidelines-use-default-member-init
+  ` to
+  :doc:`modernize-use-default-member-init
+  ` was added.
+
 Changes in existing checks
 ^^
 - Improved :doc:`readability-redundant-string-cstr
@@ -215,6 +220,11 @@
 - Deprecated :doc:`cert-dcl21-cpp
   ` check.
 
+- Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
+  `. Please use
+  :doc:`cppcoreguidelines-use-default-member-init
+  ` instead.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   ` 

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Carlos Galvez via Phabricator via cfe-commits]

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

Rebase and fix conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148460

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst

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
@@ -207,6 +207,7 @@
`cppcoreguidelines-rvalue-reference-param-not-moved `_,
`cppcoreguidelines-slicing `_,
`cppcoreguidelines-special-member-functions `_,
+   `cppcoreguidelines-use-default-member-init `_, "Yes"
`cppcoreguidelines-virtual-class-destructor `_, "Yes"
`darwin-avoid-spinlock `_,
`darwin-dispatch-once-nonstatic `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-use-default-member-init
+.. meta::
+   :http-equiv=refresh: 5;URL=../modernize/use-default-member-init.html
+
+cppcoreguidelines-use-default-member-init
+=
+
+This check implements `C.48 `_ from the CppCoreGuidelines.
+
+The cppcoreguidelines-use-default-member-init check is an alias, please see
+`modernize-use-default-member-init <../modernize/use-default-member-init.html>`_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
@@ -23,6 +23,13 @@
 initializations already implemented as member initializers. For that purpose
 see check `modernize-use-default-member-init <../modernize/use-default-member-init.html>`_.
 
+.. note::
+
+  Enforcement of rule C.48 in this check is deprecated, to be removed in
+  :program:`clang-tidy` version 19 (only C.49 will be enforced by this check then).
+  Please use `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+  to enfoce rule C.48.
+
 Example 1
 -
 
@@ -85,6 +92,11 @@
 
 .. option:: UseAssignment
 
+   Note: this option is deprecated, to be removed in :program:`clang-tidy`
+   version 19. Please use the `UseAssignment` option from
+   `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+   instead.
+
If this option is set to `true` (by default `UseAssignment` from
`modernize-use-default-member-init
<../modernize/use-default-member-init.html>`_ will be used),
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -174,6 +174,11 @@
   ` to :doc:`bugprone-unsafe-functions
   ` was added.
 
+- New alias :doc:`cppcoreguidelines-use-default-member-init
+  ` to
+  :doc:`modernize-use-default-member-init
+  ` was added.
+
 Changes in existing checks
 ^^
 - Improved :doc:`readability-redundant-string-cstr
@@ -215,6 +220,11 @@
 - Deprecated :doc:`cert-dcl21-cpp
   ` check.
 
+- Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
+  `. Please use
+  :doc:`cppcoreguidelines-use-default-member-init
+  ` instead.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   ` check.
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
@@ -12,6 +12,7 @@
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
+#include "../modernize/UseDefaultMemberInitCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include 

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Carlos Galvez via Phabricator via cfe-commits]

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

Revert code removal, document deprecation notice instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148460

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst

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
@@ -207,6 +207,7 @@
`cppcoreguidelines-rvalue-reference-param-not-moved `_,
`cppcoreguidelines-slicing `_,
`cppcoreguidelines-special-member-functions `_,
+   `cppcoreguidelines-use-default-member-init `_, "Yes"
`cppcoreguidelines-virtual-class-destructor `_, "Yes"
`darwin-avoid-spinlock `_,
`darwin-dispatch-once-nonstatic `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-use-default-member-init
+.. meta::
+   :http-equiv=refresh: 5;URL=../modernize/use-default-member-init.html
+
+cppcoreguidelines-use-default-member-init
+=
+
+This check implements `C.48 `_ from the CppCoreGuidelines.
+
+The cppcoreguidelines-use-default-member-init check is an alias, please see
+`modernize-use-default-member-init <../modernize/use-default-member-init.html>`_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
@@ -23,6 +23,13 @@
 initializations already implemented as member initializers. For that purpose
 see check `modernize-use-default-member-init <../modernize/use-default-member-init.html>`_.
 
+.. note::
+
+  Enforcement of rule C.48 in this check is deprecated, to be removed in
+  :program:`clang-tidy` version 19 (only C.49 will be enforced by this check then).
+  Please use `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+  to enfoce rule C.48.
+
 Example 1
 -
 
@@ -85,6 +92,11 @@
 
 .. option:: UseAssignment
 
+   Note: this option is deprecated, to be removed in :program:`clang-tidy`
+   version 19. Please use the `UseAssignment` option from
+   `cppcoreguidelines-use-default-member-init <../cppcoreguidelines/use-default-member-init.html>`_
+   instead.
+
If this option is set to `true` (default is `false`), the check will initialize
members with an assignment. In this case the fix of the first example looks
like this:
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -174,6 +174,11 @@
   ` to :doc:`bugprone-unsafe-functions
   ` was added.
 
+- New alias :doc:`cppcoreguidelines-use-default-member-init
+  ` to
+  :doc:`modernize-use-default-member-init
+  ` was added.
+
 Changes in existing checks
 ^^
 - Improved :doc:`readability-redundant-string-cstr
@@ -215,6 +220,11 @@
 - Deprecated :doc:`cert-dcl21-cpp
   ` check.
 
+- Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
+  `. Please use
+  :doc:`cppcoreguidelines-use-default-member-init
+  ` instead.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   ` check.
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
@@ -12,6 +12,7 @@
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
+#include "../modernize/UseDefaultMemberInitCheck.h"
 #include 

[PATCH] D148460: [clang-tidy] Add alias cppcoreguidelines-use-default-member-init

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, jeroen.dobbelaere, shchenz, kbarton, 
xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

And remove identical functionality from
cppcoreguidelines-prefer-member-initializer, which had too many
responsibilities and a tight coupling to the
modernize-use-default-member-init check.

Fixes https://github.com/llvm/llvm-project/issues/62164.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148460

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/prefer-member-initializer.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/use-default-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init-assignment.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init.cpp
+++ /dev/null
@@ -1,30 +0,0 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-prefer-member-initializer,modernize-use-default-member-init %t
-
-class Simple1 {
-  int n;
-  // CHECK-FIXES: int n{0};
-  double x;
-  // CHECK-FIXES: double x{0.0};
-
-public:
-  Simple1() {
-n = 0;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-x = 0.0;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'x' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-  }
-
-  Simple1(int nn, double xx) {
-// CHECK-FIXES: Simple1(int nn, double xx) : n(nn), x(xx) {
-n = nn;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-x = xx;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'x' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-  }
-
-  ~Simple1() = default;
-};
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/prefer-member-initializer-modernize-use-default-member-init-assignment.cpp
+++ /dev/null
@@ -1,31 +0,0 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-prefer-member-initializer,modernize-use-default-member-init %t -- \
-// RUN: -config="{CheckOptions: [{key: modernize-use-default-member-init.UseAssignment, value: true}]}"
-
-class Simple1 {
-  int n;
-  // CHECK-FIXES: int n = 0;
-  double x;
-  // CHECK-FIXES: double x = 0.0;
-
-public:
-  Simple1() {
-n = 0;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-x = 0.0;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'x' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-  }
-
-  Simple1(int nn, double xx) {
-// CHECK-FIXES: Simple1(int nn, double xx) : n(nn), x(xx) {
-n = nn;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'n' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-x = xx;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'x' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
-// CHECK-FIXES: {{^\ *$}}
-  }
-
-  ~Simple1() = default;
-};
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
@@ -207,6 +207,7 @@
`cppcoreguidelines-rvalue-reference-param-not-moved `_,
`cppcoreguidelines-slicing `_,

[PATCH] D147929: [clang-tidy] Fix handling of UseAssignment option in cppcoreguidelines-prefer-member-initializer

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a subscriber: aaron.ballman.
carlosgalvezp added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp:134
+  Options.get("UseAssignment",
+  OptionsView("modernize-use-default-member-init",
+  Context->getOptions().CheckOptions, Context)

PiotrZSL wrote:
> carlosgalvezp wrote:
> > This is very strange, feels like it's done to ensure the checks are in sync 
> > but IMO it creates more harm than good and makes the check harder to 
> > maintain. The checks are independent anyway (not aliases), so I believe it 
> > makes sense to keep being independent also in the options.
> > 
> > There are similar checks (e.g. magic numbers) where the user needs to 
> > either only enable one of the checks, or enter the same configuration 
> > settings twice.
> > 
> > I would vote for just treating this like an independent argument like all 
> > other checks, to avoid bugs like these.
> So remove dependency on modernize-use-default-member-init completly, or just 
> on modernize-use-default-member-init options ?
I had a look at the original patch:

https://reviews.llvm.org/D71199

There's quite some discussion about the topic, and reviewers like 
@aaron.ballman weren't very happy about introducing this coupling. Seems like 
the problem is non-trivial so I vote for leaving your patch as is, which fixes 
a Github ticket and brings value, and think about completely removing the 
dependency to modernize-use-default-member-init on a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147929

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

[PATCH] D145865: [clang-tidy] Multiple fixes to bugprone-exception-escape

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

There's a lot of changes in this patch (at least 6 as per commit message) - 
please split into smaller patches fixing 1 problem at a time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145865

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

[PATCH] D144036: [clang-tidy] Add bugprone-non-zero-enum-to-bool-conversion check

[Carlos Galvez via Phabricator via cfe-commits]

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

LGTM, thanks! Would be good to fix the last nit before landing.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.h:17
+
+///  Warns about `enum` to `bool` conversions where `enum` doesn't have a
+///  zero-value enumerator.

Nit remove excessive whitespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144036

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

[PATCH] D98416: [clang-tidy] Fix cppcoreguidelines-narrowing-conversions false positive

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Took the liberty to land it for you, hope it's ok! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98416

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

[PATCH] D98416: [clang-tidy] Fix cppcoreguidelines-narrowing-conversions false positive

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4530c3bc4897: [clang-tidy] Fix 
cppcoreguidelines-narrowing-conversions false positive (authored by njames93, 
committed by carlosgalvezp).

Changed prior to commit:
  https://reviews.llvm.org/D98416?vs=329929=513912#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98416

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp
@@ -54,4 +54,11 @@
   f = __builtin_nan("0"); // double NaN is not narrowing.
 }
 
+double false_positive_const_qualified_cast(bool t) {
+  double b = 1.0;
+  constexpr double a = __builtin_huge_val();
+  // PR49498 The constness difference of 'a' and 'b' results in an implicit 
cast.
+  return t ? b : a;
+}
+
 } // namespace floats
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -510,6 +510,8 @@
   const BuiltinType *RhsType = getBuiltinType(Rhs);
   if (RhsType == nullptr || LhsType == nullptr)
 return;
+  if (LhsType == RhsType)
+return;
   if (RhsType->getKind() == BuiltinType::Bool && LhsType->isSignedInteger())
 return handleBooleanToSignedIntegral(Context, SourceLoc, Lhs, Rhs);
   if (RhsType->isInteger() && LhsType->getKind() == BuiltinType::Bool)
@@ -549,6 +551,8 @@
   const Expr  = *Cast.getSubExpr();
   if (Lhs.isInstantiationDependent() || Rhs.isInstantiationDependent())
 return;
+  if (getBuiltinType(Lhs) == getBuiltinType(Rhs))
+return;
   if (handleConditionalOperator(Context, Lhs, Rhs))
 return;
   SourceLocation SourceLoc = Lhs.getExprLoc();


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-narrowingfloatingpoint-option.cpp
@@ -54,4 +54,11 @@
   f = __builtin_nan("0"); // double NaN is not narrowing.
 }
 
+double false_positive_const_qualified_cast(bool t) {
+  double b = 1.0;
+  constexpr double a = __builtin_huge_val();
+  // PR49498 The constness difference of 'a' and 'b' results in an implicit cast.
+  return t ? b : a;
+}
+
 } // namespace floats
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -510,6 +510,8 @@
   const BuiltinType *RhsType = getBuiltinType(Rhs);
   if (RhsType == nullptr || LhsType == nullptr)
 return;
+  if (LhsType == RhsType)
+return;
   if (RhsType->getKind() == BuiltinType::Bool && LhsType->isSignedInteger())
 return handleBooleanToSignedIntegral(Context, SourceLoc, Lhs, Rhs);
   if (RhsType->isInteger() && LhsType->getKind() == BuiltinType::Bool)
@@ -549,6 +551,8 @@
   const Expr  = *Cast.getSubExpr();
   if (Lhs.isInstantiationDependent() || Rhs.isInstantiationDependent())
 return;
+  if (getBuiltinType(Lhs) == getBuiltinType(Rhs))
+return;
   if (handleConditionalOperator(Context, Lhs, Rhs))
 return;
   SourceLocation SourceLoc = Lhs.getExprLoc();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148340: [clang-tidy] Apply cppcoreguidelines-avoid-capture-default-when-capturin-this only to by-value capture default

[Carlos Galvez via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h:1
+//===--- MisleadingCaptureDefaultByValueCheck.h - clang-tidy*- C++
+//-*-===//

Eugene.Zelenko wrote:
> Split header comment.
Thanks, fixed in an NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148340

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

[PATCH] D148424: [clang-tidy][NFC] Improve doc of cppcoreguidelines-misleading-capture-default-by-value

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfa3de2ed2964: [clang-tidy][NFC] Improve doc of 
cppcoreguidelines-misleading-capture-default… (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148424

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst


Index: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
===
--- 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
+++ 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
@@ -5,10 +5,14 @@
 
 Warns when lambda specify a by-value capture default and capture ``this``.
 
-By-value capture-defaults in member functions can be misleading about
-whether data members are captured by value or reference. For example,
-specifying the capture default ``[=]`` will still capture data members
-by reference.
+By-value capture defaults in member functions can be misleading about whether
+data members are captured by value or reference. This occurs because specifying
+the capture default ``[=]`` actually captures the ``this`` pointer by value,
+not the data members themselves. As a result, data members are still indirectly
+accessed via the captured ``this`` pointer, which essentially means they are
+being accessed by reference. Therefore, even when using ``[=]``, data members
+are effectively captured by reference, which might not align with the user's
+expectations.
 
 Examples:
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,11 +112,6 @@
   This check relies heavily on, but is not exclusive to, the functions from
   the *Annex K. "Bounds-checking interfaces"* of C11.
 
-- New :doc:`cppcoreguidelines-misleading-capture-default-by-value
-  ` 
check.
-
-  Warns when lambda specify a by-value capture default and capture ``this``.
-
 - New :doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines
   ` 
check.
 
@@ -124,6 +119,11 @@
   use-after-free errors and suggests avoiding captures or ensuring the lambda
   closure object has a guaranteed lifetime.
 
+- New :doc:`cppcoreguidelines-misleading-capture-default-by-value
+  ` 
check.
+
+  Warns when lambda specify a by-value capture default and capture ``this``.
+
 - New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved
   ` 
check.
 
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
@@ -16,11 +16,8 @@
 
 /// Warns when lambda specify a by-value capture default and capture ``this``.
 ///
-/// By-value capture defaults in lambas defined within member functions can be
-/// misleading about whether capturing data member is by value or reference.
-/// For example, [=] will capture local variables by value but member variables
-/// by reference. CppCoreGuideline F.54 suggests to never use by-value capture
-/// default when capturing this.
+/// By-value capture defaults in member functions can be misleading about
+/// whether data members are captured by value or reference.
 ///
 /// For the user-facing documentation see:
 /// 
http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.html


Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
@@ -5,10 +5,14 @@
 
 Warns when lambda specify a by-value capture default and capture ``this``.
 
-By-value capture-defaults in member functions can be misleading about
-whether data members are captured by value or reference. For example,
-specifying the capture default ``[=]`` will still capture data members
-by reference.
+By-value capture defaults in member functions can be misleading about whether
+data members are captured by value or reference. This occurs because specifying
+the capture default ``[=]`` actually captures the ``this`` pointer by value,
+not the data members themselves. As a result, data members are still 

[PATCH] D148418: [clang-tidy] Fix typedefs handling in bugprone-dangling-handle

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp:55-56
 
-  operator basic_string_view() const noexcept;
+  typedef basic_string_view str_view;
+  operator str_view() const noexcept;
 

PiotrZSL wrote:
> carlosgalvezp wrote:
> > Right now the test will no longer test classes that have the conversion 
> > operator written explicitly instead of via typedef. Do you think it's worth 
> > keeping the other implementation as well, or can we safely assume that 
> > `basic_string` is always implemented like this?
> Both GCC (__sv_type) & Clang (__self_view) implement this using typedef.
> But code will work fine also without typedef.
> 
> But code will work fine also without typedef.

I guess we can't know if this will hold in the future unless we have an 
explicit test for it :) But I agree it's probably unlikely and if we had 
implemented this from scratch copying the pattern from GCC or Clang directly we 
would have ended up with this result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148418

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

[PATCH] D148424: [clang-tidy][NFC] Improve doc of cppcoreguidelines-misleading-capture-default-by-value

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, shchenz, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Also fix ordering in Release Notes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148424

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst


Index: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
===
--- 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
+++ 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
@@ -5,10 +5,14 @@
 
 Warns when lambda specify a by-value capture default and capture ``this``.
 
-By-value capture-defaults in member functions can be misleading about
-whether data members are captured by value or reference. For example,
-specifying the capture default ``[=]`` will still capture data members
-by reference.
+By-value capture defaults in member functions can be misleading about whether
+data members are captured by value or reference. This occurs because specifying
+the capture default ``[=]`` actually captures the ``this`` pointer by value,
+not the data members themselves. As a result, data members are still indirectly
+accessed via the captured ``this`` pointer, which essentially means they are
+being accessed by reference. Therefore, even when using ``[=]``, data members
+are effectively captured by reference, which might not align with the user's
+expectations.
 
 Examples:
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,11 +112,6 @@
   This check relies heavily on, but is not exclusive to, the functions from
   the *Annex K. "Bounds-checking interfaces"* of C11.
 
-- New :doc:`cppcoreguidelines-misleading-capture-default-by-value
-  ` 
check.
-
-  Warns when lambda specify a by-value capture default and capture ``this``.
-
 - New :doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines
   ` 
check.
 
@@ -124,6 +119,11 @@
   use-after-free errors and suggests avoiding captures or ensuring the lambda
   closure object has a guaranteed lifetime.
 
+- New :doc:`cppcoreguidelines-misleading-capture-default-by-value
+  ` 
check.
+
+  Warns when lambda specify a by-value capture default and capture ``this``.
+
 - New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved
   ` 
check.
 
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
@@ -16,11 +16,8 @@
 
 /// Warns when lambda specify a by-value capture default and capture ``this``.
 ///
-/// By-value capture defaults in lambas defined within member functions can be
-/// misleading about whether capturing data member is by value or reference.
-/// For example, [=] will capture local variables by value but member variables
-/// by reference. CppCoreGuideline F.54 suggests to never use by-value capture
-/// default when capturing this.
+/// By-value capture defaults in member functions can be misleading about
+/// whether data members are captured by value or reference.
 ///
 /// For the user-facing documentation see:
 /// 
http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.html


Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
@@ -5,10 +5,14 @@
 
 Warns when lambda specify a by-value capture default and capture ``this``.
 
-By-value capture-defaults in member functions can be misleading about
-whether data members are captured by value or reference. For example,
-specifying the capture default ``[=]`` will still capture data members
-by reference.
+By-value capture defaults in member functions can be misleading about whether
+data members are captured by value or reference. This occurs because specifying
+the capture default ``[=]`` actually captures the ``this`` pointer by value,
+not 

[PATCH] D148340: [clang-tidy] Apply cppcoreguidelines-avoid-capture-default-when-capturin-this only to by-value capture default

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGeedbe81b1c6d: [clang-tidy] Apply 
cppcoreguidelines-avoid-capture-default-when-capturin-this… (authored by 
carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148340

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/MisleadingCaptureDefaultByValueCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/misleading-capture-default-by-value.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/misleading-capture-default-by-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/misleading-capture-default-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/misleading-capture-default-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/misleading-capture-default-by-value.cpp
@@ -1,7 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t \
-// RUN: -check-suffixes=,DEFAULT
-// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t \
-// RUN: -config="{CheckOptions: [{key: cppcoreguidelines-avoid-capture-default-when-capturing-this.IgnoreCaptureDefaultByReference, value: true}]}"
+// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-misleading-capture-default-by-value %t
 
 struct Obj {
   void lambdas_that_warn_default_capture_copy() {
@@ -9,73 +6,60 @@
 int local2{};
 
 auto explicit_this_capture = [=, this]() { };
-// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that capture 'this' should not specify a by-value capture default [cppcoreguidelines-misleading-capture-default-by-value]
 // CHECK-FIXES: auto explicit_this_capture = [this]() { };
 
 auto explicit_this_capture_locals1 = [=, this]() { return (local+x) > 10; };
-// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a by-value capture default [cppcoreguidelines-misleading-capture-default-by-value]
 // CHECK-FIXES: auto explicit_this_capture_locals1 = [local, this]() { return (local+x) > 10; };
 
 auto explicit_this_capture_locals2 = [=, this]() { return (local+local2) > 10; };
-// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a by-value capture default [cppcoreguidelines-misleading-capture-default-by-value]
 // CHECK-FIXES: auto explicit_this_capture_locals2 = [local, local2, this]() { return (local+local2) > 10; };
 
 auto explicit_this_capture_local_ref = [=, this, ]() { return (local+x) > 10; };
-// CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+// CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a by-value capture default [cppcoreguidelines-misleading-capture-default-by-value]
 // CHECK-FIXES: auto explicit_this_capture_local_ref = [this, ]() { return (local+x) > 10; };
 
 auto explicit_this_capture_local_ref2 = [=, , this]() { return (local+x) > 10; };
-// CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+// CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a by-value capture default [cppcoreguidelines-misleading-capture-default-by-value]
 

[PATCH] D148418: [clang-tidy] Fix typedefs handling in bugprone-dangling-handle

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp:55-56
 
-  operator basic_string_view() const noexcept;
+  typedef basic_string_view str_view;
+  operator str_view() const noexcept;
 

Right now the test will no longer test classes that have the conversion 
operator written explicitly instead of via typedef. Do you think it's worth 
keeping the other implementation as well, or can we safely assume that 
`basic_string` is always implemented like this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148418

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