[PATCH] D126735: [clang-tidy] Silence modernize-redundant-void-arg in the case of vexing parses

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.
Herald added a reviewer: njames93.
Herald added a subscriber: PiotrZSL.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp:596
+
+// Explicitly specifying `(void)` is a way to sidestep -Wvexing-parse, so we
+// should not warn about it here.

This is not the only way to eliminate the vexing parse warning.  For instance, 
if the intention is to declare `foo` as a function not in this translation 
unit, then:

```
void most_vexing_parse() {
  extern int foo();
}
```
does the trick.

Why assume that the only way to fix vexing-parse is to add `(void)`?


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

https://reviews.llvm.org/D126735

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

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a subscriber: PiotrZSL.

In D128697#3638467 , @Sockke wrote:

> In D128697#3619310 , 
> @LegalizeAdulthood wrote:
>
>> This whole check seems weird to me.  I mean, almost every use of a standard 
>> container could throw `std::bad_alloc` but we don't insist on a local 
>> `catch` for `bad_alloc` at every possible throwing call site.
>>
>> Why would we assume that every call site of `stoi` or `stod` **must** have 
>> an exception handler immediately around it?  It's perfectly acceptable for 
>> an application to handle this at an outer scope that can't be detected by 
>> clang-tidy.
>
> Makes sense, I implemented this check here because some projects in ByteDance 
> used stoi with missing exception handlers caused an online crash, I think 
> this is a relatively common problem.  Perhaps only report diagnostics for 
> stoi calls in nothrow functions?

I don't have anything more to add; I mean, you answered my question with 
another question.  I still don't see that this check is making things better.  
Have you run it on large code bases to see what kind of nuisance it creates?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D133102: [clang-tidy] Extend simplify-boolean-expr check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a subscriber: PiotrZSL.

In D133102#3774151 , @alexfh wrote:

> For example, LLVM coding standards recommends using early exits: 
> https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

How is the proposed replacement not an early exit?  It's an unconditional exit: 
it either returns a bool constant or it returns a variable that is a bool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133102

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

[PATCH] D146655: [clang-tidy] Ignore DISABLED_ in test suite name in google-avoid-underscore-in-googletest-name

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google/avoid-underscore-in-googletest-name.cpp:90
 // Underscores are allowed to disable a test with the DISABLED_ prefix.
-// 
https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
+// 
https://google.github.io/googletest/faq.html#why-should-test-suite-names-and-test-names-not-contain-underscore
 TEST(TestCaseName, TestName) {}

PiotrZSL wrote:
> Would be good if this link would exist also in documentation for this check.
My experience has been that these external links move around with gtest and are 
a source of churn.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146655

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

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a subscriber: PiotrZSL.

I'm not certain that the result of the transformation is more "readable"; is 
this check intended to aid conformance to a style guide?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130181

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

[PATCH] D130630: [clang-tidy] Add readability-nested-ifs check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a subscriber: PiotrZSL.

How does this check interact with macros that expand to `if` statements?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130630

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

[PATCH] D72940: Add a support for clang tidy to import another configurations files from .clang-tidy

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood resigned from this revision.
LegalizeAdulthood added a comment.
This revision now requires review to proceed.
Herald added a reviewer: njames93.
Herald added a subscriber: PiotrZSL.
Herald added a project: All.

No action taken, removing myself as a reviewer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72940

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

[PATCH] D134590: [clang-tidy] Fix a false positive in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood 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/D134590/new/

https://reviews.llvm.org/D134590

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

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf7a80c3d08d4: [clang-tidy] Properly forward clang-tidy 
output when running tests (authored by nicovank, committed by 
LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127807

Files:
  clang-tools-extra/test/clang-tidy/check_clang_tidy.py


Index: clang-tools-extra/test/clang-tidy/check_clang_tidy.py
===
--- clang-tools-extra/test/clang-tidy/check_clang_tidy.py
+++ clang-tools-extra/test/clang-tidy/check_clang_tidy.py
@@ -173,13 +173,13 @@
 print('Running ' + repr(args) + '...')
 clang_tidy_output = try_run(args)
 print(' clang-tidy output ---')
-print(clang_tidy_output.encode())
-
print('\n--')
+print(clang_tidy_output.encode(sys.stdout.encoding, 
errors="replace").decode(sys.stdout.encoding))
+print('--')
 
 diff_output = try_run(['diff', '-u', self.original_file_name, 
self.temp_file_name], False)
-print('-- Fixes 
-\n' +
-  diff_output +
-  
'\n--')
+print('-- Fixes -')
+print(diff_output)
+print('--')
 return clang_tidy_output
 
   def check_fixes(self):


Index: clang-tools-extra/test/clang-tidy/check_clang_tidy.py
===
--- clang-tools-extra/test/clang-tidy/check_clang_tidy.py
+++ clang-tools-extra/test/clang-tidy/check_clang_tidy.py
@@ -173,13 +173,13 @@
 print('Running ' + repr(args) + '...')
 clang_tidy_output = try_run(args)
 print(' clang-tidy output ---')
-print(clang_tidy_output.encode())
-print('\n--')
+print(clang_tidy_output.encode(sys.stdout.encoding, errors="replace").decode(sys.stdout.encoding))
+print('--')
 
 diff_output = try_run(['diff', '-u', self.original_file_name, self.temp_file_name], False)
-print('-- Fixes -\n' +
-  diff_output +
-  '\n--')
+print('-- Fixes -')
+print(diff_output)
+print('--')
 return clang_tidy_output
 
   def check_fixes(self):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I will submit it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127807

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa65a3bffd31f: [clang-tidy] Don't treat invalid branches 
as identical (authored by ishaangandhi, committed by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128402

Files:
  clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s bugprone-branch-clone %t
+
+int test_unknown_expression() {
+  if (unknown_expression_1) {// CHECK-MESSAGES: :[[@LINE]]:7: error: 
use of undeclared identifier 'unknown_expression_1' [clang-diagnostic-error]
+function1(unknown_expression_2); // CHECK-MESSAGES: :[[@LINE]]:15: error: 
use of undeclared identifier 'unknown_expression_2' [clang-diagnostic-error]
+  } else {
+function2(unknown_expression_3); // CHECK-MESSAGES: :[[@LINE]]:15: error: 
use of undeclared identifier 'unknown_expression_3' [clang-diagnostic-error]
+  }
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -158,6 +158,10 @@
 - Fixed nonsensical suggestion of :doc:`altera-struct-pack-align
   ` check for empty structs.
 
+- Fixed a false positive in :doc:`bugprone-branch-clone
+  ` when the branches
+  involve unknown expressions.
+
 - Fixed some false positives in :doc:`bugprone-infinite-loop
   ` involving dependent expressions.
 
Index: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -19,6 +19,17 @@
 /// Returns true when the statements are Type I clones of each other.
 static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
const ASTContext &Context) {
+  if (isa(LHS) && isa(RHS)) {
+// If we have errors in expressions, we will be unable
+// to accurately profile and compute hashes for each
+// of the left and right statements.
+const auto *LHSExpr = llvm::cast(LHS);
+const auto *RHSExpr = llvm::cast(RHS);
+if (LHSExpr->containsErrors() && RHSExpr->containsErrors()) {
+  return false;
+}
+  }
+
   llvm::FoldingSetNodeID DataLHS, DataRHS;
   LHS->Profile(DataLHS, Context, false);
   RHS->Profile(DataRHS, Context, false);


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s bugprone-branch-clone %t
+
+int test_unknown_expression() {
+  if (unknown_expression_1) {// CHECK-MESSAGES: :[[@LINE]]:7: error: use of undeclared identifier 'unknown_expression_1' [clang-diagnostic-error]
+function1(unknown_expression_2); // CHECK-MESSAGES: :[[@LINE]]:15: error: use of undeclared identifier 'unknown_expression_2' [clang-diagnostic-error]
+  } else {
+function2(unknown_expression_3); // CHECK-MESSAGES: :[[@LINE]]:15: error: use of undeclared identifier 'unknown_expression_3' [clang-diagnostic-error]
+  }
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -158,6 +158,10 @@
 - Fixed nonsensical suggestion of :doc:`altera-struct-pack-align
   ` check for empty structs.
 
+- Fixed a false positive in :doc:`bugprone-branch-clone
+  ` when the branches
+  involve unknown expressions.
+
 - Fixed some false positives in :doc:`bugprone-infinite-loop
   ` involving dependent expressions.
 
Index: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -19,6 +19,17 @@
 /// Returns true when the statements are Type I clones of each other.
 static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
const ASTContext &Context) {
+  if (isa(LHS) && isa(RHS)) {
+// If we have errors in expressions, we will be unable
+// to accurately profile and c

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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:130
 
+- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  ` check.

Sort new checks by check name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:131
+- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  ` check.
+

This link is wrong, please install Sphinx and build the target 
`docs-clang-tools-html` to validate links


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D128157#3612741 , @myhsu wrote:

> You forgot to add 
> 
>  "Differential Revison: https://reviews.llvm.org/DXX"; in the commit 
> message, which is how Phabricator identifies the differential to close. I 
> don't think it has anything to do with rebasing.

Oh, OK, that makes sense, oops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
 
+- Improved :doc:`bugprone-signal-handler
+  ` check. Partial

sort by check name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

[PATCH] D128511: [clang-tidy] Make the cert/uppercase-literal-suffix-integer fully hermetic.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/integral_constant.h:1
+#ifndef _INTEGRAL_CONSTANT_H_
+#define _INTEGRAL_CONSTANT_H_

Identifiers beginning with `_` followed by an uppercase letter are [[ 
https://en.cppreference.com/w/cpp/language/identifiers | reserved for the 
implementation ]].

Include guards should never include double underscores or begin with an 
underscore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128511

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

[PATCH] D127807: [clang-tidy] Properly forward clang-tidy output when running tests

[Richard via Phabricator via cfe-commits]

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

Thanks for this, like Nathan James this has been pestering me for a while `:)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127807

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Please provide the name and email address you wish to use on the commit and I 
will submit.


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

https://reviews.llvm.org/D128402

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I can submit after you address additional comments by Nathan James.


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

https://reviews.llvm.org/D128402

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

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

[Richard via Phabricator via cfe-commits]

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

Clang-tidy tests and docs have been moved to subdirectories by module, please 
rebase to `main:HEAD`


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

https://reviews.llvm.org/D54943

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[Richard via Phabricator via cfe-commits]

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

Please add a summary of the fix to the release notes for this check.


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

https://reviews.llvm.org/D127293

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

[PATCH] D128697: [clang-tidy] Add new check `bugprone-unhandled-exception-at-sto`

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

This whole check seems weird to me.  I mean, almost every use of a standard 
container could throw `std::bad_alloc` but we don't insist on a local `catch` 
for `bad_alloc` at every possible throwing call site.

Why would we assume that every call site of `stoi` or `stod` **must** have an 
exception handler immediately around it?  It's perfectly acceptable for an 
application to handle this at an outer scope that can't be detected by 
clang-tidy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128697

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

[PATCH] D128715: [clang-tidy] Fix confusable identifiers interaction with DeclContext

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/confusable-identifiers.cpp:43
+  return q0 < q1;
+}

aaron.ballman wrote:
> It looks like there's quite a bit of missing test coverage for all the 
> various situations in C++. Here are some additional test cases to add:
> ```
> template 
> struct S {
>   template  // Should warn on this one, right?
>   void func2() {}
> };
> 
> template 
> void func(int il) { // Should warn on this one, right?
> }
> 
> template 
> void other() {
>   int OO = 0; // Should warn on this one, right?
> }
> int OO = 0; // But not this one
> 
> namespace foo {
>   int i1;
> }
> 
> namespace bar {
>   int il;  // Should not warn about this
> }
> 
> namespace foo {
>   int il; // But should warn about this
> }
> 
> struct Base {
>   virtual void O0();
> 
> private:
>   void II();
> };
> 
> struct Derived : Base {
>   void OO(); // We should warn about this
>   void I1(); // But not this
> };
> ```
> (I'm sure there are more that I've missed, but you get the idea.) If any of 
> these point out other issues, feel free to address them in a follow-up patch 
> if you'd prefer.
Another case to consider:
```
template 
void f();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128715

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

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

I can submit after you address additional comments by Nathan James.


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

https://reviews.llvm.org/D128402

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I pushed this change, but for some reason phabricator doesn't show it and close 
the review... I wonder if it's because I rebased it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D128402

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

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

Well, it must have been unrelated to your change or some weird interaction on 
my machine because I rebased and ran `check-clang-extra` again and it passed 
this time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:282
+// These are all class names in ExprCXX.h without 'CXX' prefix.
+#define CXXStmts \
+  ArrayTypeTraitExpr,\

I'm not a fan of this macro, and we're only using it once as the argument to 
`isa<>`; is there some problem with writing it inline?



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:246
 
+- Improved :doc:`bugprone-signal-handler
+  ` check. Partial

Please keep the section sorted by check, so this should be inserted before 
`bugprone-use-after-move`



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/signal-handler.cpp:1
+// RUN: %check_clang_tidy -std=c++14 %s bugprone-signal-handler %t -- -- 
-isystem %S/../Inputs/Headers -isystem %S/Inputs/signal-handler
+

Prefer `-isystem %clang_tidy_headers`  instead of `-isystem 
%S/../Inputs/Headers`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

[PATCH] D128372: [Clang-Tidy] Empty Check

[Richard via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-standalone-empty.cpp:1
+// RUN: %check_clang_tidy %s bugprone-standalone-empty %t
+

This file should be removed from the changelist


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128372

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

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

Rebase against main to get updated docs


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

https://reviews.llvm.org/D128402

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

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

I get a test failure when I run with your change:

-- Testing: 972 tests, 12 workers --
FAIL: Clang Tools :: clang-apply-replacements/basic.cpp (680 of 972)
 TEST 'Clang Tools :: 
clang-apply-replacements/basic.cpp' FAILED 
Script:
--
: 'RUN: at line 1';   mkdir -p 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
: 'RUN: at line 2';   grep -Ev "// *[A-Z-]+:" 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h
 > 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
: 'RUN: at line 3';   sed 
"s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file1.yaml
 > 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file1.yaml
: 'RUN: at line 4';   sed 
"s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file2.yaml
 > 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file2.yaml
: 'RUN: at line 5';   clang-apply-replacements 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
: 'RUN: at line 6';   FileCheck 
-input-file=D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h
: 'RUN: at line 9';   ls -1 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
 | FileCheck 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp
 --check-prefix=YAML
: 'RUN: at line 12';   grep -Ev "// *[A-Z-]+:" 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h
 > 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
: 'RUN: at line 13';   clang-apply-replacements -remove-change-desc-files 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
: 'RUN: at line 14';   ls -1 
D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
 | FileCheck 
D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp
 --check-prefix=NO_YAML
--
Exit Code: 1
  
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "mkdir" "-p" 
"D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ ":" "RUN: at line 2"
$ "grep" "-Ev" "// *[A-Z-]+:" 
"D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h"
$ ":" "RUN: at line 3"
$ "sed" 
"s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
"D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file1.yaml"
$ ":" "RUN: at line 4"
$ "sed" 
"s#\$(path)#D:/legalize/llvm/llvm-project/build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
"D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/file2.yaml"
$ ":" "RUN: at line 5"
$ "clang-apply-replacements" 
"D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ ":" "RUN: at line 6"
$ "FileCheck" 
"-input-file=D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h"
 
"D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h"
$ ":" "RUN: at line 9"
$ "ls" "-1" 
"D:\legalize\llvm\llvm-project\build\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ "FileCheck" 
"D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements\basic.cpp"
 "--check-prefix=YAML"
$ ":" "RUN: at line 12"
$ "grep" "-Ev" "// *[A-Z-]+:" 
"D:\legalize\llvm\llvm-project\clang-tools-extra\test\clang-apply-replacements/Inputs/basic/basic.h"
$ ":" "RUN: at line 13"
$ "clang-apply-replacements" "-remove-change-desc-files" 
"D:\legalize\llvm\llvm-project\b

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D128157#3605829 , @jspam wrote:

> I see :) Yes, looks like Git's rename detection did its job.
>
> Thanks for the review. Could you or someone else please merge this for me? 
> Author: Joachim Priesner 

I can submit, yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D128401: [clang-tidy] Fixing a bug raising false alarms on static local variables in the Infinite Loop Checker

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp:190
+
+containsFunc |= (CanDecl == Func);
+overlap |= Callees.contains(CanDecl);

Personally I'm not a fan of using bitwise operators with booleans.  It involves 
implicit casts of bool to int and int to bool and doesn't use the usual 
short-circuiting logic of `||` and `&&`.  It also means this code won't be 
analyzed by clang-tidy as "performing operations on booleans" for things like 
`readability-simplify-boolean-expr`.


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

https://reviews.llvm.org/D128401

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

OK, I've pushed the link fix and the sorting fix to main so if you rebase 
again, it should just show your changes to the release notes.


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

https://reviews.llvm.org/D128402

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:221-224
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  ` when the 
specialization
+  template has an unnecessary value parameter. Removed the fix for a template.
+

I'm confused.  The link text says `performance-unnecessary-value-param`, but 
the link is to `readability-suspicious-call-argument` and neither appears to be 
an alias for the other.

It appears this mistake was already there and you just alphabetized the list.  
I'm going to fix it in main and alphabetize the list so you can rebase and just 
add your changes in isolation.


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

https://reviews.llvm.org/D128402

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood 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/D128157/new/

https://reviews.llvm.org/D128157

___
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()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
Herald added a project: All.

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rCTE Clang Tools Extra

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] D116577: [clang-tidy] Added "boost-use-range-based-for-loop" check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
Herald added a project: All.

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


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

https://reviews.llvm.org/D116577

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

[PATCH] D126247: [clang-tidy][doc] Document readability-indentifier-naming resolution order and examples

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126247

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D128157#3605504 , @jspam wrote:

> @LegalizeAdulthood Not sure what exactly you mean by "fold your changes into 
> `checkers/cppcoreguidelines/virtual-class-destructor.cpp`". The only rebase 
> conflict was in `VirtualClassDestructorCheck.cpp`. Please let me know if 
> there is still something to do.

Clang-Tidy tests and docs have moved to module subdirectories.  You had 
originally edited the file 
`checkers/cppcoreguidelines-virtual-class-destructor.cpp` (no subdirectory, 
file named with module prefix) and this file was moved to 
`checkers/cppcoreguidelines/virtual-class-destructor.cpp` (now in a module 
subdirectory, no module prefix in file name).

Looks like git handled it automatically and you didn't notice `:)`.  This is 
good because it means I did the "right git thing" when I moved/renamed the 
files and that for most people, simply rebasing should be enough to ensure that 
their changes are following the file to the new location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


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

https://reviews.llvm.org/D128314

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

[PATCH] D128372: Clang-Tidy Empty Check

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128372

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

[PATCH] D128401: [clang-tidy] Fixing a bug raising false alarms on static local variables in the Infinite Loop Checker

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128401

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

[PATCH] D128402: [clang-tidy] Don't treat invalid branches as identical

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:237
+- Fixed a false positive in :doc:`bugprone-branch-clone
+  ` when the branches
+  involve unknown expressions.

This link is wrong, needs to use the `bugprone/` directory.

Make the target `docs-clang-tools-html` to verify that all your links are 
correct.


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

https://reviews.llvm.org/D128402

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

[PATCH] D117593: [clang-tidy] Change google-explicit-constructor to ignore conversions and operators marked `explicit(false)`

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.
Herald added a project: All.

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117593

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

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118743

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

[PATCH] D126097: [clang-tidy] Adds the NSDateFormatter checker to clang-tidy

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126097

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

[PATCH] D126735: [clang-tidy] Silence modernize-redundant-void-arg in the case of vexing parses

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126735

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

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124918

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

[PATCH] D127036: [clang-tidy] Warn about arrays in `bugprone-undefined-memory-manipulation`

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


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

https://reviews.llvm.org/D127036

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

[PATCH] D127293: [clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


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

https://reviews.llvm.org/D127293

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

[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Richard via Phabricator via cfe-commits]

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

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996

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

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

[Richard via Phabricator via cfe-commits]

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

Tests and docs have moved to subdirectories by module name.

Please rebase onto HEAD and:

- move 
`test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp`
 to 
`test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp`
- move 
`docs/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.rst` 
to 
`docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst`
- make the target `test-clang-extra` to validate your tests still pass
- make the target `docs-clang-tools-html` to validate your documentation links 
are correct


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

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

[PATCH] D128157: [clang-tidy] cppcoreguidelines-virtual-class-destructor: Fix crash when "virtual" keyword is expanded from a macro

[Richard via Phabricator via cfe-commits]

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

Please rebase this and folder your changes into 
`checkers/cppcoreguidelines/virtual-class-destructor.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128157

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

[PATCH] D128072: [clang-tidy] Organize test files into subdirectories by module (NFC)

[Richard 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 rG89a1d03e2b37: [clang-tidy] Organize test files into 
subdirectories by module (NFC) (authored by LegalizeAdulthood).

Changed prior to commit:
  https://reviews.llvm.org/D128072?vs=437963&id=439102#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128072

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/external-file.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/flags/internal-file.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/time/time.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/kernel.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/verilog.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/vhdl.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-c.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-cxx.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-reserved-identifier/system/system-header.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-reserved-identifier/user-header.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/google-namespaces.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/gtest.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/nosuite/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/nosuite/gtest/gtest.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/resource/include/stdatomic.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/resource/include/stddef.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/stdio.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/system/string.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/assert.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/complex.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/ctype.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/errno.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/fenv.h
  
clang-tools-extra

[PATCH] D128072: [clang-tidy] Organize test files into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D128072#3598634 , @aaron.ballman 
wrote:

> This sounds like a sensible direction to me. I spot-checked the changes and 
> they all seem reasonable. Giving my LG, but please wait a bit before landing 
> in case someone else has the chance to spot check as well.

Testing was:

- check-clang-extra passed
- add_new_check.py added the boiler plate test code in the correct place


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128072

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

[PATCH] D128072: [clang-tidy] Organize test files into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added reviewers: njames93, aaron.ballman.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: bzcheeseman, carlosgalvezp, abrachet, lebedev.ri, 
jdoerfert, arphaman, zzheng, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: lebedev.ri.
Herald added a reviewer: lebedev.ri.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.
Herald added a subscriber: aheejin.

Eliminate clutter by reorganizing the Lit test files for clang-tidy:

- Move checkers/-* to checkers//*.
- Move module specific inputs from Inputs to /Inputs.  Remove any 
module prefix from the file or subdirectory name as they are no longer needed.
- Introduce a Lit substitution %clang_tidy_headers for the system headers in 
checkers/Inputs/Headers and use this throughout.  This avoids referencing 
system headers through a relative path to the parent directory and makes it 
clear that these fake system headers are shared among all modules.
- Update add_new_check.py to follow the above conventions when creating the 
boiler plate test files for a new check.
- Update Contributing.rst to describe per-module Inputs directory and fix link 
to test source code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128072

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/external-file.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/flags/internal-file.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/strings/internal-file.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/time/time.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/kernel.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/verilog.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/vhdl.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-c.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-cxx.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-reserved-identifier/system/system-header.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-reserved-identifier/user-header.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/google-namespaces.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/gtest.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/nosuite/gtest/gtest-typed-test.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/gtest/nosuite/gtest/gtest.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/resource/include/stdatomic.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/resource/include/st

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D126495#3588787 , @aaron.ballman 
wrote:

> In D126495#3574115 , 
> @LegalizeAdulthood wrote:
>
>> In D126495#3571511 , @njames93 
>> wrote:
>>
>>> Search engines isn't really an issue, its more people who are using older 
>>> versions of llvm that try and go to the documentation which turns out is a 
>>> dead link. 
>>> Having said that, I still think the lack of a redirect shouldn't block this 
>>> change.
>>
>> Aren't we archiving old versions of the documentation, though?
>
> We do:
>
> https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/
> https://releases.llvm.org/11.0.0/tools/clang/tools/extra/docs/
> and so on.
>
> Giving my LG more officially now to unblock this, but I'd still appreciate 
> some extra eyes on the Python changes.

I tested by generating a new check and validating the links in the 
documentation as well as running `--update-docs` and checking that the updates 
were correct.  So functionally, there should be no problem and I tried to 
follow the existing style of the file, so should be good on style nits as well.


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Ping.  All review comments should be addressed in the latest diff.


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D126495#3571511 , @njames93 wrote:

> Search engines isn't really an issue, its more people who are using older 
> versions of llvm that try and go to the documentation which turns out is a 
> dead link. 
> Having said that, I still think the lack of a redirect shouldn't block this 
> change.

Aren't we archiving old versions of the documentation, though?

So if I know I'm using LLVM 3.8, I would consult the documentation for 3.8 not 
top-of-tree, and all the links would work correctly within the 3.8 
documentation, adjusting for the base path.


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D126495#3570767 , @aaron.ballman 
wrote:

> In D126495#3569863 , 
> @LegalizeAdulthood wrote:
>
>> In D126495#3569029 , @njames93 
>> wrote:
>>
>>> It would also be nice if there was a redirect that would dynamically 
>>> translate the old links to new links.
>>
>> You can do that with `.htaccess`, but I don't know if that's considered 
>> acceptable in clang documentation.
>
> FWIW, I don't have any idea if this is or isn't acceptable.

Mucking around with an `.htaccess` file is very web server dependent.  Surely 
this can be the first time that a page of documentation changed names or 
locations?  I'm betting that they didn't worry about putting in redirects and 
let search engines handle guiding users to the correct page.


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

https://reviews.llvm.org/D126495

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

[PATCH] D118711: [hack] Build a tree of preprocessing directives

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a project: All.

I'm discarding this review, as it was just for comment purposes, until I have a 
proper implementation to discuss.


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

https://reviews.llvm.org/D118711

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D126495#3569029 , @njames93 wrote:

> In D126495#3568998 , 
> @LegalizeAdulthood wrote:
>
>> Gentle ping
>
> My previous point about the links in the header files not being updated 
> hasn't been addressed.

Ah yes, I got distracted and forgot about that.  I will update the diff.

> It would also be nice if there was a redirect that would dynamically 
> translate the old links to new links.

You can do that with `.htaccess`, but I don't know if that's considered 
acceptable in clang documentation.

Leaving `.rst` pages in place for all the old locations just recreates the 
clutter instead of getting rid of it.


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Gentle ping


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D126495#3553183 , 
@LegalizeAdulthood wrote:

> In D126495#3552887 , @njames93 
> wrote:
>
>> Can I ask what the motivation is for this change?
>
> https://discourse.llvm.org/t/clang-tidy-rfc-use-module-name-as-a-directory-for-organizing-files/62499

Basically there are a crapton of files in a single directory, making for 
clutter.

Additionally, on Windows, TAB completion of filenames in cmd shells doesn't 
stop at the first non-unique character in a path, so it's essentially useless 
when a crapton of files all begin with the same prefix.  If the files are 
grouped by module, then I can TAB complete on the module and usually the checks 
differ after a few characters in the check name once the module prefix is 
stripped, so typing a few characters and then TAB completes to the desired file 
or a few more TABs to cycle through the remaining matching files to get the one 
I want.


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D126495#3552887 , @njames93 wrote:

> Can I ask what the motivation is for this change?

https://discourse.llvm.org/t/clang-tidy-rfc-use-module-name-as-a-directory-for-organizing-files/62499


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

https://reviews.llvm.org/D126495

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

[PATCH] D126495: [clang-tidy] Organize test docs into subdirectories by module (NFC)

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

I know this is a rather huge diff, but the part that needs to be reviewed is 
the python script changes.

I've rebuilt the HTML docs with sphinx and tested all the manually modified 
links to verify that they still go to the correct page.

The existing Release Notes have been rebased as well.


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

https://reviews.llvm.org/D126495

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

[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Richard 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 rGb418ef5cb90b: [clang-tidy] Reject invalid enum initializers 
in C files (authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125622

Files:
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
  clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
@@ -36,177 +36,241 @@
   return Tokens;
 }
 
-static bool matchText(const char *Text) {
+static bool matchText(const char *Text, bool AllowComma) {
   std::vector Tokens{tokenify(Text)};
-  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens);
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, AllowComma);
 
   return Matcher.match();
 }
 
+static modernize::LiteralSize sizeText(const char *Text) {
+  std::vector Tokens{tokenify(Text)};
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, true);
+  if (Matcher.match())
+return Matcher.largestLiteralSize();
+  return modernize::LiteralSize::Unknown;
+}
+
+static const char *toString(modernize::LiteralSize Value) {
+  switch (Value) {
+  case modernize::LiteralSize::Int:
+return "Int";
+  case modernize::LiteralSize::UnsignedInt:
+return "UnsignedInt";
+  case modernize::LiteralSize::Long:
+return "Long";
+  case modernize::LiteralSize::UnsignedLong:
+return "UnsignedLong";
+  case modernize::LiteralSize::LongLong:
+return "LongLong";
+  case modernize::LiteralSize::UnsignedLongLong:
+return "UnsignedLongLong";
+  default:
+return "Unknown";
+  }
+}
+
 namespace {
 
-struct Param {
+struct MatchParam {
+  bool AllowComma;
   bool Matched;
   const char *Text;
 
-  friend std::ostream &operator<<(std::ostream &Str, const Param &Value) {
-return Str << "Matched: " << std::boolalpha << Value.Matched << ", Text: '"
-   << Value.Text << "'";
+  friend std::ostream &operator<<(std::ostream &Str, const MatchParam &Value) {
+return Str << "Allow operator,: " << std::boolalpha << Value.AllowComma
+   << ", Matched: " << std::boolalpha << Value.Matched
+   << ", Text: '" << Value.Text << '\'';
   }
 };
 
-class MatcherTest : public ::testing::TestWithParam {};
+struct SizeParam {
+  modernize::LiteralSize Size;
+  const char *Text;
+
+  friend std::ostream &operator<<(std::ostream &Str, const SizeParam &Value) {
+return Str << "Size: " << toString(Value.Size) << ", Text: '" << Value.Text << '\'';
+  }
+};
+
+class MatcherTest : public ::testing::TestWithParam {};
+
+class SizeTest : public ::testing::TestWithParam {};
 
 } // namespace
 
-static const Param Params[] = {
+static const MatchParam MatchParams[] = {
 // Accept integral literals.
-{true, "1"},
-{true, "0177"},
-{true, "0xdeadbeef"},
-{true, "0b1011"},
-{true, "'c'"},
+{true, true, "1"},
+{true, true, "0177"},
+{true, true, "0xdeadbeef"},
+{true, true, "0b1011"},
+{true, true, "'c'"},
 // Reject non-integral literals.
-{false, "1.23"},
-{false, "0x1p3"},
-{false, R"("string")"},
-{false, "1i"},
+{true, false, "1.23"},
+{true, false, "0x1p3"},
+{true, false, R"("string")"},
+{true, false, "1i"},
 
 // Accept literals with these unary operators.
-{true, "-1"},
-{true, "+1"},
-{true, "~1"},
-{true, "!1"},
+{true, true, "-1"},
+{true, true, "+1"},
+{true, true, "~1"},
+{true, true, "!1"},
 // Reject invalid unary operators.
-{false, "1-"},
-{false, "1+"},
-{false, "1~"},
-{false, "1!"},
+{true, false, "1-"},
+{true, false, "1+"},
+{true, false, "1~"},
+{true, false, "1!"},
 
 // Accept valid binary operators.
-{true, "1+1"},
-{true, "1-1"},
-{true, "1*1"},
-{true, "1/1"},
-{true, "1%2"},
-{true, "1<<1"},
-{true, "1>>1"},
-{true, "1<=>1"},
-{true, "1<1"},
-{true, "1>1"},
-{true, "1<=1"},
-{true, "1>=1"},
-{true, "1==1"},
-{true, "1!=1"},
-{true, "1&1"},
-{true, "1^1"},
-{true, "1|1"},
-{true, "1&&1"},
-{true, "1||1"},
-{true, "1+ +1"}, // A space is needed to avoid being tokenized as ++ or --.
-{true, "1- -1"},
-{true, "1,1"},
+{true, true, "1+1"},
+{true, true, "1-1"},
+{tru

[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 433648.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D125622

Files:
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
  clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
@@ -36,177 +36,241 @@
   return Tokens;
 }
 
-static bool matchText(const char *Text) {
+static bool matchText(const char *Text, bool AllowComma) {
   std::vector Tokens{tokenify(Text)};
-  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens);
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, AllowComma);
 
   return Matcher.match();
 }
 
+static modernize::LiteralSize sizeText(const char *Text) {
+  std::vector Tokens{tokenify(Text)};
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, true);
+  if (Matcher.match())
+return Matcher.largestLiteralSize();
+  return modernize::LiteralSize::Unknown;
+}
+
+static const char *toString(modernize::LiteralSize Value) {
+  switch (Value) {
+  case modernize::LiteralSize::Int:
+return "Int";
+  case modernize::LiteralSize::UnsignedInt:
+return "UnsignedInt";
+  case modernize::LiteralSize::Long:
+return "Long";
+  case modernize::LiteralSize::UnsignedLong:
+return "UnsignedLong";
+  case modernize::LiteralSize::LongLong:
+return "LongLong";
+  case modernize::LiteralSize::UnsignedLongLong:
+return "UnsignedLongLong";
+  default:
+return "Unknown";
+  }
+}
+
 namespace {
 
-struct Param {
+struct MatchParam {
+  bool AllowComma;
   bool Matched;
   const char *Text;
 
-  friend std::ostream &operator<<(std::ostream &Str, const Param &Value) {
-return Str << "Matched: " << std::boolalpha << Value.Matched << ", Text: '"
-   << Value.Text << "'";
+  friend std::ostream &operator<<(std::ostream &Str, const MatchParam &Value) {
+return Str << "Allow operator,: " << std::boolalpha << Value.AllowComma
+   << ", Matched: " << std::boolalpha << Value.Matched
+   << ", Text: '" << Value.Text << '\'';
   }
 };
 
-class MatcherTest : public ::testing::TestWithParam {};
+struct SizeParam {
+  modernize::LiteralSize Size;
+  const char *Text;
+
+  friend std::ostream &operator<<(std::ostream &Str, const SizeParam &Value) {
+return Str << "Size: " << toString(Value.Size) << ", Text: '" << Value.Text << '\'';
+  }
+};
+
+class MatcherTest : public ::testing::TestWithParam {};
+
+class SizeTest : public ::testing::TestWithParam {};
 
 } // namespace
 
-static const Param Params[] = {
+static const MatchParam MatchParams[] = {
 // Accept integral literals.
-{true, "1"},
-{true, "0177"},
-{true, "0xdeadbeef"},
-{true, "0b1011"},
-{true, "'c'"},
+{true, true, "1"},
+{true, true, "0177"},
+{true, true, "0xdeadbeef"},
+{true, true, "0b1011"},
+{true, true, "'c'"},
 // Reject non-integral literals.
-{false, "1.23"},
-{false, "0x1p3"},
-{false, R"("string")"},
-{false, "1i"},
+{true, false, "1.23"},
+{true, false, "0x1p3"},
+{true, false, R"("string")"},
+{true, false, "1i"},
 
 // Accept literals with these unary operators.
-{true, "-1"},
-{true, "+1"},
-{true, "~1"},
-{true, "!1"},
+{true, true, "-1"},
+{true, true, "+1"},
+{true, true, "~1"},
+{true, true, "!1"},
 // Reject invalid unary operators.
-{false, "1-"},
-{false, "1+"},
-{false, "1~"},
-{false, "1!"},
+{true, false, "1-"},
+{true, false, "1+"},
+{true, false, "1~"},
+{true, false, "1!"},
 
 // Accept valid binary operators.
-{true, "1+1"},
-{true, "1-1"},
-{true, "1*1"},
-{true, "1/1"},
-{true, "1%2"},
-{true, "1<<1"},
-{true, "1>>1"},
-{true, "1<=>1"},
-{true, "1<1"},
-{true, "1>1"},
-{true, "1<=1"},
-{true, "1>=1"},
-{true, "1==1"},
-{true, "1!=1"},
-{true, "1&1"},
-{true, "1^1"},
-{true, "1|1"},
-{true, "1&&1"},
-{true, "1||1"},
-{true, "1+ +1"}, // A space is needed to avoid being tokenized as ++ or --.
-{true, "1- -1"},
-{true, "1,1"},
+{true, true, "1+1"},
+{true, true, "1-1"},
+{true, true, "1*1"},
+{true, true, "1/1"},
+{true, true, "1%2"},
+{true, true, "1<<1"},
+{true, true, "1>>1"},
+{true, true, "1<=>1"},
+{true, true, "1<

[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:3-7
+// C requires enum values to fit into an int.
+#define TOO_BIG1 1L
+#define TOO_BIG2 1UL
+#define TOO_BIG3 1LL
+#define TOO_BIG4 1ULL

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > How do we want to handle the fact that Clang has an extension to allow 
> > > this? Perhaps we want a config option for pedantic vs non-pedantic fixes?
> > I was trying to find a way to make it fail on gcc/clang on compiler 
> > explorer and I couldn't get it to fail in either compiler
> > 
> > Where is the extension documented?  It appears to be on by default in both 
> > compilers.
> I don't think we have any documentation for it (we basically don't bother to 
> document our implementation-defined behavior, which drives me mildly nuts -- 
> we fall back on "the source code is the definitive documentation", which is 
> not user-friendly at all).
> 
> That's why I was wondering if we wanted a pedantic vs non-pedantic option 
> here. Pedantically, the behavior you have today is correct. However, because 
> this is a super common extension to compilers, we might want to allow it to 
> be transformed despite being pedantically an extension.
> 
> We can handle that as a follow-up though.
Is there a way to force it to fail with some compiler flag?  If you could show 
me on compiler explorer, that would be great.


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

https://reviews.llvm.org/D125622

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

[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 432676.
LegalizeAdulthood added a comment.

- Add C++ test case for acceptable `operator,` initializer


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

https://reviews.llvm.org/D125622

Files:
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
  clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
@@ -36,177 +36,241 @@
   return Tokens;
 }
 
-static bool matchText(const char *Text) {
+static bool matchText(const char *Text, bool AllowComma) {
   std::vector Tokens{tokenify(Text)};
-  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens);
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, AllowComma);
 
   return Matcher.match();
 }
 
+static modernize::LiteralSize sizeText(const char *Text) {
+  std::vector Tokens{tokenify(Text)};
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, true);
+  if (Matcher.match())
+return Matcher.largestLiteralSize();
+  return modernize::LiteralSize::Unknown;
+}
+
+static const char *toString(modernize::LiteralSize Value) {
+  switch (Value) {
+  case modernize::LiteralSize::Int:
+return "Int";
+  case modernize::LiteralSize::UnsignedInt:
+return "UnsignedInt";
+  case modernize::LiteralSize::Long:
+return "Long";
+  case modernize::LiteralSize::UnsignedLong:
+return "UnsignedLong";
+  case modernize::LiteralSize::LongLong:
+return "LongLong";
+  case modernize::LiteralSize::UnsignedLongLong:
+return "UnsignedLongLong";
+  default:
+return "Unknown";
+  }
+}
+
 namespace {
 
-struct Param {
+struct MatchParam {
+  bool AllowComma;
   bool Matched;
   const char *Text;
 
-  friend std::ostream &operator<<(std::ostream &Str, const Param &Value) {
-return Str << "Matched: " << std::boolalpha << Value.Matched << ", Text: '"
-   << Value.Text << "'";
+  friend std::ostream &operator<<(std::ostream &Str, const MatchParam &Value) {
+return Str << "Allow operator,: " << std::boolalpha << Value.AllowComma
+   << ", Matched: " << std::boolalpha << Value.Matched
+   << ", Text: '" << Value.Text << '\'';
   }
 };
 
-class MatcherTest : public ::testing::TestWithParam {};
+struct SizeParam {
+  modernize::LiteralSize Size;
+  const char *Text;
+
+  friend std::ostream &operator<<(std::ostream &Str, const SizeParam &Value) {
+return Str << "Size: " << toString(Value.Size) << ", Text: '" << Value.Text << '\'';
+  }
+};
+
+class MatcherTest : public ::testing::TestWithParam {};
+
+class SizeTest : public ::testing::TestWithParam {};
 
 } // namespace
 
-static const Param Params[] = {
+static const MatchParam MatchParams[] = {
 // Accept integral literals.
-{true, "1"},
-{true, "0177"},
-{true, "0xdeadbeef"},
-{true, "0b1011"},
-{true, "'c'"},
+{true, true, "1"},
+{true, true, "0177"},
+{true, true, "0xdeadbeef"},
+{true, true, "0b1011"},
+{true, true, "'c'"},
 // Reject non-integral literals.
-{false, "1.23"},
-{false, "0x1p3"},
-{false, R"("string")"},
-{false, "1i"},
+{true, false, "1.23"},
+{true, false, "0x1p3"},
+{true, false, R"("string")"},
+{true, false, "1i"},
 
 // Accept literals with these unary operators.
-{true, "-1"},
-{true, "+1"},
-{true, "~1"},
-{true, "!1"},
+{true, true, "-1"},
+{true, true, "+1"},
+{true, true, "~1"},
+{true, true, "!1"},
 // Reject invalid unary operators.
-{false, "1-"},
-{false, "1+"},
-{false, "1~"},
-{false, "1!"},
+{true, false, "1-"},
+{true, false, "1+"},
+{true, false, "1~"},
+{true, false, "1!"},
 
 // Accept valid binary operators.
-{true, "1+1"},
-{true, "1-1"},
-{true, "1*1"},
-{true, "1/1"},
-{true, "1%2"},
-{true, "1<<1"},
-{true, "1>>1"},
-{true, "1<=>1"},
-{true, "1<1"},
-{true, "1>1"},
-{true, "1<=1"},
-{true, "1>=1"},
-{true, "1==1"},
-{true, "1!=1"},
-{true, "1&1"},
-{true, "1^1"},
-{true, "1|1"},
-{true, "1&&1"},
-{true, "1||1"},
-{true, "1+ +1"}, // A space is needed to avoid being tokenized as ++ or --.
-{true, "1- -1"},
-{true, "1,1"},
+{true, true, "1+1"},
+{true, true, "1-1"},
+{true, true, "1*1"},
+{true, true, "1/1"},
+{true, true, "1%2"},
+{true, true, "1<<1"},
+{true, true, "1>>1"},
+{true, true, 

[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Richard via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:324
 {
-  IntegralLiteralExpressionMatcher Matcher(MacroTokens);
-  return Matcher.match();
+  IntegralLiteralExpressionMatcher Matcher(MacroTokens, LangOpts.C99 == 0);
+  bool Matched = Matcher.match();

aaron.ballman wrote:
> Huh? Comma wasn't allowed in C89 either... In fact, there's no language mode 
> which allows top-level commas in either C or C++ -- the issue with the comma 
> operator is a parsing one. You can't tell the difference between the comma 
> being part of the initializer expression or the comma being the separator 
> between enumerators.
The most recent diff handles the issue with `operator,` but the previous 
version was only handling the overly-big literal



Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:326
+  bool Matched = Matcher.match();
+  if (LangOpts.C99 &&
+  (Matcher.largestLiteralSize() != LiteralSize::Int &&

aaron.ballman wrote:
> And C89?
`C99` was the earliest dialect of C I could find in `LangOptions.def`.  If you 
can show me an earlier version for C, I'm happy to switch it.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:3-7
+// C requires enum values to fit into an int.
+#define TOO_BIG1 1L
+#define TOO_BIG2 1UL
+#define TOO_BIG3 1LL
+#define TOO_BIG4 1ULL

aaron.ballman wrote:
> How do we want to handle the fact that Clang has an extension to allow this? 
> Perhaps we want a config option for pedantic vs non-pedantic fixes?
I was trying to find a way to make it fail on gcc/clang on compiler explorer 
and I couldn't get it to fail in either compiler

Where is the extension documented?  It appears to be on by default in both 
compilers.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:10
+// C forbids comma operator in initializing expressions.
+#define BAD_OP 1, 2
+

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > Can you add a test:
> > > ```
> > > #define GOOD_OP (1, 2)
> > > ```
> > > to make sure it still gets converted to an enum?
> > Yeah, another one I was thinking of is when someone does [[ 
> > https://godbolt.org/z/c677je8s1 | something as disgusting as this. ]]
> > 
> > ```
> > #define ARGS 1, 2
> > #define OTHER_ARGS (1, 2)
> > 
> > int f(int x, int y) { return x + y; }
> > 
> > int main()
> > {
> > return f(ARGS) + f OTHER_ARGS;
> > }
> > ```
> > 
> > However, the only real way to handle avoiding conversion of the 2nd case is 
> > to examine the context of macro expansion.  This is another edge case that 
> > will have to be handled subsequently.
> > 
> > This gets tricky because AFAIK there is no way to select expressions in the 
> > AST that result from macro expansion.  You have to match the macro 
> > expansion locations against AST nodes to identify the node(s) that match 
> > the expansion location yourself.
> That's a good test case (for some definition of good)! :-)
> 
> At this point, there's a few options:
> 
> 1) Go back to the way things were -- disallow comma expressions, even in 
> parens. Document it as a limitation.
> 2) Accept comma expressions in parens, ignore the problem case like you 
> described above. Document it as a limitation?
> 3) Try to solve every awful code construct we can ever imagine. Document the 
> check is perfect in every way, but probably not land it for several years.
> 
> I think I'd be happy with #2 but I could also live with #1
In the most recent diff, I'm supporting `operator,` inside parens for C++ and 
never allowing it in C.

I plan to do a subsequent enhancement that examines the expansion locations of 
macros and rejects any macro whose expansion doesn't encompass a single 
expression.  In other words, if the tokens in the macro somehow get 
incorporated into some syntactic element that isn't completely encased in a 
single expression, then reject the macro.


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

https://reviews.llvm.org/D125622

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

[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 432675.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

- Update from review comments
- Disallow operator, unless inside parentheses


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

https://reviews.llvm.org/D125622

Files:
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp
  clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h
  clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c
  clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp
  clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp
@@ -36,177 +36,241 @@
   return Tokens;
 }
 
-static bool matchText(const char *Text) {
+static bool matchText(const char *Text, bool AllowComma) {
   std::vector Tokens{tokenify(Text)};
-  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens);
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, AllowComma);
 
   return Matcher.match();
 }
 
+static modernize::LiteralSize sizeText(const char *Text) {
+  std::vector Tokens{tokenify(Text)};
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, true);
+  if (Matcher.match())
+return Matcher.largestLiteralSize();
+  return modernize::LiteralSize::Unknown;
+}
+
+static const char *toString(modernize::LiteralSize Value) {
+  switch (Value) {
+  case modernize::LiteralSize::Int:
+return "Int";
+  case modernize::LiteralSize::UnsignedInt:
+return "UnsignedInt";
+  case modernize::LiteralSize::Long:
+return "Long";
+  case modernize::LiteralSize::UnsignedLong:
+return "UnsignedLong";
+  case modernize::LiteralSize::LongLong:
+return "LongLong";
+  case modernize::LiteralSize::UnsignedLongLong:
+return "UnsignedLongLong";
+  default:
+return "Unknown";
+  }
+}
+
 namespace {
 
-struct Param {
+struct MatchParam {
+  bool AllowComma;
   bool Matched;
   const char *Text;
 
-  friend std::ostream &operator<<(std::ostream &Str, const Param &Value) {
-return Str << "Matched: " << std::boolalpha << Value.Matched << ", Text: '"
-   << Value.Text << "'";
+  friend std::ostream &operator<<(std::ostream &Str, const MatchParam &Value) {
+return Str << "Allow operator,: " << std::boolalpha << Value.AllowComma
+   << ", Matched: " << std::boolalpha << Value.Matched
+   << ", Text: '" << Value.Text << '\'';
   }
 };
 
-class MatcherTest : public ::testing::TestWithParam {};
+struct SizeParam {
+  modernize::LiteralSize Size;
+  const char *Text;
+
+  friend std::ostream &operator<<(std::ostream &Str, const SizeParam &Value) {
+return Str << "Size: " << toString(Value.Size) << ", Text: '" << Value.Text << '\'';
+  }
+};
+
+class MatcherTest : public ::testing::TestWithParam {};
+
+class SizeTest : public ::testing::TestWithParam {};
 
 } // namespace
 
-static const Param Params[] = {
+static const MatchParam MatchParams[] = {
 // Accept integral literals.
-{true, "1"},
-{true, "0177"},
-{true, "0xdeadbeef"},
-{true, "0b1011"},
-{true, "'c'"},
+{true, true, "1"},
+{true, true, "0177"},
+{true, true, "0xdeadbeef"},
+{true, true, "0b1011"},
+{true, true, "'c'"},
 // Reject non-integral literals.
-{false, "1.23"},
-{false, "0x1p3"},
-{false, R"("string")"},
-{false, "1i"},
+{true, false, "1.23"},
+{true, false, "0x1p3"},
+{true, false, R"("string")"},
+{true, false, "1i"},
 
 // Accept literals with these unary operators.
-{true, "-1"},
-{true, "+1"},
-{true, "~1"},
-{true, "!1"},
+{true, true, "-1"},
+{true, true, "+1"},
+{true, true, "~1"},
+{true, true, "!1"},
 // Reject invalid unary operators.
-{false, "1-"},
-{false, "1+"},
-{false, "1~"},
-{false, "1!"},
+{true, false, "1-"},
+{true, false, "1+"},
+{true, false, "1~"},
+{true, false, "1!"},
 
 // Accept valid binary operators.
-{true, "1+1"},
-{true, "1-1"},
-{true, "1*1"},
-{true, "1/1"},
-{true, "1%2"},
-{true, "1<<1"},
-{true, "1>>1"},
-{true, "1<=>1"},
-{true, "1<1"},
-{true, "1>1"},
-{true, "1<=1"},
-{true, "1>=1"},
-{true, "1==1"},
-{true, "1!=1"},
-{true, "1&1"},
-{true, "1^1"},
-{true, "1|1"},
-{true, "1&&1"},
-{true, "1||1"},
-{true, "1+ +1"}, // A space is needed to avoid being tokenized as ++ or --.
-{true, "1- -1"},
-{true, "1,1"},
+{true, true, "1+1"},
+{true, true, "1-1"},
+{true, true, "1*1"},
+{true, true, "1/1"},
+{true, true, "1%2"},
+ 

[PATCH] D126247: `readability-indentifier-naming` resolution order and examples

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2742
 double   dValueDouble = 0.0;
 ULONGulValueUlong = 0;

Phabricator says there is no context available.  Did you generate this diff 
with `git diff -U99 main`?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2750
+The full set of identifier classifications listed above includes some overlap,
+where an individual identifier can falling into several classifications. Bellow
+is a list of the classifications supported by ``readability-identifier-naming``

`fall`, not `falling`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2750
+The full set of identifier classifications listed above includes some overlap,
+where an individual identifier can falling into several classifications. Bellow
+is a list of the classifications supported by ``readability-identifier-naming``

LegalizeAdulthood wrote:
> `fall`, not `falling`
`Below`, not `Bellow`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2756
+matched. This occurs when the semantics of the identifier match and there is a
+valid option for the classification present in the ``.clang-tidy`` file - e.g.,
+``readability-identifier-naming.AbstractClassCase``.

Does it have to be in the `.clang-tidy` file, or is it just a matter of setting 
options?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2772
+   - Class ``[class, struct]``
+   - Struct ``[class]``
+   - Union ``[union]``

Why is `Struct`  listed twice?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2786
+ - 
+   - ConstexprVariable ``[constexpr]``
+   - ``[const]``

I would have expected `ConstExprVariable` instead?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2857
+attempt disambiguation within the context of this document. For example,
+ identifiers that clang tidy is currently looking only at
+member variables.

Eugene.Zelenko wrote:
> Ditto.
`identifies`, not `identifiers`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2873
+classify the identifier in any other context. For example, in the 
+ semantic context, clang tidy will abort matching after failing
+to resolve the ``Parameter`` classification and a parameter will *not* be

`parameters` not `paramaters`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2873
+classify the identifier in any other context. For example, in the 
+ semantic context, clang tidy will abort matching after failing
+to resolve the ``Parameter`` classification and a parameter will *not* be

LegalizeAdulthood wrote:
> `parameters` not `paramaters`
Clang-tidy, not clang tidy



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2880
+
+The code snippet below[1]_ serves as an exhaustive example of various
+identifiers the ``readability-identifier-naming`` check is able to classify.

Should we be linking to ephemeral gists that can disappear?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3054
+// @param param Parameter
+int func(std::string *const str_ptr, const std::string &str, int 
*ptr_param, int param)
+{

This covers "const pointer", but what about "pointer to const"?
e.g. `std::string const *str_ptr` how is this handled?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3120
+// GlobalConstantPointer, GlobalConstant, Constant, GlobalPointer, 
GlobalVariable, Variable
+int *const global_const_ptr = nullptr;
+

Same, pointer to `const`?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3127-3129
+// StaticConstant does not actually trip for this declaration despite the 
documentation indicating that it should.
+// StaticConstant does not appear to trip for anything. Reading the code, 
it seems that StaticConstant logic is in the
+// wrong place and the conditions cannot be met.

Is this really a bug report disguised as a doc comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126247

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https:/

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard 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 rG89e663c4f83a: [clang-tidy] Improve add_new_check.py to 
recognize more checks (authored by LegalizeAdulthood).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126134

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  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
@@ -37,19 +37,19 @@
`altera-struct-pack-align `_, "Yes"
`altera-unroll-loops `_,
`android-cloexec-accept `_, "Yes"
-   `android-cloexec-accept4 `_,
+   `android-cloexec-accept4 `_, "Yes"
`android-cloexec-creat `_, "Yes"
`android-cloexec-dup `_, "Yes"
-   `android-cloexec-epoll-create `_,
-   `android-cloexec-epoll-create1 `_,
-   `android-cloexec-fopen `_,
-   `android-cloexec-inotify-init `_,
-   `android-cloexec-inotify-init1 `_,
-   `android-cloexec-memfd-create `_,
-   `android-cloexec-open `_,
+   `android-cloexec-epoll-create `_, "Yes"
+   `android-cloexec-epoll-create1 `_, "Yes"
+   `android-cloexec-fopen `_, "Yes"
+   `android-cloexec-inotify-init `_, "Yes"
+   `android-cloexec-inotify-init1 `_, "Yes"
+   `android-cloexec-memfd-create `_, "Yes"
+   `android-cloexec-open `_, "Yes"
`android-cloexec-pipe `_, "Yes"
-   `android-cloexec-pipe2 `_,
-   `android-cloexec-socket `_,
+   `android-cloexec-pipe2 `_, "Yes"
+   `android-cloexec-socket `_, "Yes"
`android-comparison-in-temp-failure-retry `_,
`boost-use-to-string `_, "Yes"
`bugprone-argument-comment `_, "Yes"
@@ -105,7 +105,7 @@
`bugprone-terminating-continue `_, "Yes"
`bugprone-throw-keyword-missing `_,
`bugprone-too-small-loop-variable `_,
-   `bugprone-unchecked-optional-access `_, "Yes"
+   `bugprone-unchecked-optional-access `_,
`bugprone-undefined-memory-manipulation `_,
`bugprone-undelegated-constructor `_,
`bugprone-unhandled-exception-at-new `_,
Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -158,12 +158,17 @@
'namespace': namespace})
 
 
-# Modifies the module to include the new check.
-def adapt_module(module_path, module, check_name, check_name_camel):
+# Returns the source filename that implements the module.
+def get_module_filename(module_path, module):
   modulecpp = list(filter(
   lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
   os.listdir(module_path)))[0]
-  filename = os.path.join(module_path, modulecpp)
+  return os.path.join(module_path, modulecpp)
+
+
+# Modifies the module to include the new check.
+def adapt_module(module_path, module, check_name, check_name_camel):
+  filename = get_module_filename(module_path, module)
   with io.open(filename, 'r', encoding='utf8') as f:
 lines = f.readlines()
 
@@ -320,24 +325,100 @@
  os.listdir(docs_dir)))
   doc_files.sort()
 
+  # We couldn't find the source file from the check name, so try to find the
+  # class name that corresponds to the check in the module file.
+  def filename_from_module(module_name, check_name):
+module_path = os.path.join(clang_tidy_path, module_name)
+if not os.path.isdir(module_path):
+  return ''
+module_file = get_module_filename(module_path, module_name)
+if not os.path.isfile(module_file):
+  return ''
+with io.open(module_file, 'r') as f:
+  code = f.read()
+  full_check_name = module_name + '-' + check_name
+  name_pos = code.find('"' + full_check_name + '"')
+  if name_pos == -1:
+return ''
+  stmt_end_pos = code.find(';', name_pos)
+  if stmt_end_pos == -1:
+return ''
+  stmt_start_pos = code.rfind(';', 0, name_pos)
+  if stmt_start_pos == -1:
+stmt_start_pos = code.rfind('{', 0, name_pos)
+  if stmt_start_pos == -1:
+return ''
+  stmt = code[stmt_start_pos+1:stmt_end_pos]
+  matches = re.search('registerCheck<([^>:]*)>\(\s*"([^"]*)"\s*\)', stmt)
+  if matches and matches[2] == full_check_name:
+class_name = matches[1]
+if '::' in class_name:
+  parts = class_name.split('::')
+  class_name = parts[-1]
+  class_path = os.path.join(clang_tidy_path, module_name, '..', *parts[0:-1])
+else:
+  class_path = os.path.join(clang_tidy_path, module_name)
+return get_actual_filename(class_path, class_name + '.cpp')
+
+return ''
+
+  # Examine code looking for a c'tor definition to get the base class name.
+  def get_base_class(code, check_file):
+check_class_name = os.path

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:335-336
+module_file = get_module_filename(module_path, module_name)
+if not os.path.isfile(module_file):
+  return ''
+with io.open(module_file, 'r') as f:

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > Do we have to check this or can we rely on open failing because it's not a 
> > file? (It tripped my psychic TOCTOU sensor.)
> If we don't check here, then it throws an exception when attempting to open 
> the file.
Also, I was following the existing pattern in this file `:)`


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

https://reviews.llvm.org/D126134

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

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 431383.
LegalizeAdulthood added a comment.

Update from review comments


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

https://reviews.llvm.org/D126134

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  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
@@ -37,19 +37,19 @@
`altera-struct-pack-align `_, "Yes"
`altera-unroll-loops `_,
`android-cloexec-accept `_, "Yes"
-   `android-cloexec-accept4 `_,
+   `android-cloexec-accept4 `_, "Yes"
`android-cloexec-creat `_, "Yes"
`android-cloexec-dup `_, "Yes"
-   `android-cloexec-epoll-create `_,
-   `android-cloexec-epoll-create1 `_,
-   `android-cloexec-fopen `_,
-   `android-cloexec-inotify-init `_,
-   `android-cloexec-inotify-init1 `_,
-   `android-cloexec-memfd-create `_,
-   `android-cloexec-open `_,
+   `android-cloexec-epoll-create `_, "Yes"
+   `android-cloexec-epoll-create1 `_, "Yes"
+   `android-cloexec-fopen `_, "Yes"
+   `android-cloexec-inotify-init `_, "Yes"
+   `android-cloexec-inotify-init1 `_, "Yes"
+   `android-cloexec-memfd-create `_, "Yes"
+   `android-cloexec-open `_, "Yes"
`android-cloexec-pipe `_, "Yes"
-   `android-cloexec-pipe2 `_,
-   `android-cloexec-socket `_,
+   `android-cloexec-pipe2 `_, "Yes"
+   `android-cloexec-socket `_, "Yes"
`android-comparison-in-temp-failure-retry `_,
`boost-use-to-string `_, "Yes"
`bugprone-argument-comment `_, "Yes"
@@ -105,7 +105,7 @@
`bugprone-terminating-continue `_, "Yes"
`bugprone-throw-keyword-missing `_,
`bugprone-too-small-loop-variable `_,
-   `bugprone-unchecked-optional-access `_, "Yes"
+   `bugprone-unchecked-optional-access `_,
`bugprone-undefined-memory-manipulation `_,
`bugprone-undelegated-constructor `_,
`bugprone-unhandled-exception-at-new `_,
Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -158,12 +158,17 @@
'namespace': namespace})
 
 
-# Modifies the module to include the new check.
-def adapt_module(module_path, module, check_name, check_name_camel):
+# Returns the source filename that implements the module.
+def get_module_filename(module_path, module):
   modulecpp = list(filter(
   lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
   os.listdir(module_path)))[0]
-  filename = os.path.join(module_path, modulecpp)
+  return os.path.join(module_path, modulecpp)
+
+
+# Modifies the module to include the new check.
+def adapt_module(module_path, module, check_name, check_name_camel):
+  filename = get_module_filename(module_path, module)
   with io.open(filename, 'r', encoding='utf8') as f:
 lines = f.readlines()
 
@@ -320,24 +325,100 @@
  os.listdir(docs_dir)))
   doc_files.sort()
 
+  # We couldn't find the source file from the check name, so try to find the
+  # class name that corresponds to the check in the module file.
+  def filename_from_module(module_name, check_name):
+module_path = os.path.join(clang_tidy_path, module_name)
+if not os.path.isdir(module_path):
+  return ''
+module_file = get_module_filename(module_path, module_name)
+if not os.path.isfile(module_file):
+  return ''
+with io.open(module_file, 'r') as f:
+  code = f.read()
+  full_check_name = module_name + '-' + check_name
+  name_pos = code.find('"' + full_check_name + '"')
+  if name_pos == -1:
+return ''
+  stmt_end_pos = code.find(';', name_pos)
+  if stmt_end_pos == -1:
+return ''
+  stmt_start_pos = code.rfind(';', 0, name_pos)
+  if stmt_start_pos == -1:
+stmt_start_pos = code.rfind('{', 0, name_pos)
+  if stmt_start_pos == -1:
+return ''
+  stmt = code[stmt_start_pos+1:stmt_end_pos]
+  matches = re.search('registerCheck<([^>:]*)>\(\s*"([^"]*)"\s*\)', stmt)
+  if matches and matches[2] == full_check_name:
+class_name = matches[1]
+if '::' in class_name:
+  parts = class_name.split('::')
+  class_name = parts[-1]
+  class_path = os.path.join(clang_tidy_path, module_name, '..', *parts[0:-1])
+else:
+  class_path = os.path.join(clang_tidy_path, module_name)
+return get_actual_filename(class_path, class_name + '.cpp')
+
+return ''
+
+  # Examine code looking for a c'tor definition to get the base class name.
+  def get_base_class(code, check_file):
+check_class_name = os.path.splitext(os.path.basename(check_file))[0]
+ctor_pattern = check_class_name + '\([^:]*\)\s*:\s*([A-Z][A-Za-z0-9]*Check)\('
+matches = re.search('\s+' + check_class_name +

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:352
+  stmt = code[stmt_start_pos+1:stmt_end_pos]
+  matches = re.search('registerCheck<([^>:]*)>\(\s*"([^"]*)"\s*\)', stmt)
+  if matches and matches[2] == full_check_name:

LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > It's a bit early for me to fully grok regex, but: is this going to handle 
> > line continuations/newlines okay? I don't know if those show up in cases 
> > that matter right now, but I wanted to make sure it was being thought about.
> `\s` matches any single whitespace character and `\s*` matches zero or more 
> whitespace characters.  I could sprinkle some more in between tokens, but 
> this catches the existing code correctly.
There is some code like this: 
`CheckFactories.registerCheck("cert-oop54-cpp");`
 and I intentionally omit searching for this (`[^:>]*`) because these are 
aliases for other checks.

Which makes me wonder if we want to explicitly register aliases differently 
from regular checks?  If you want to run a list of checks and exclude aliases 
(so the check doesn't run twice), how do you do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126134

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

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

Another observation:
If some new pattern comes up and fixits aren't recognized for a check, it might 
be better
to switch to a whitelist for checks with fixits rather than going crazier on 
the file scraping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126134

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

[PATCH] D126162: [clang-tidy] Extend SimplifyBooleanExpr demorgan support.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood 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/D126162/new/

https://reviews.llvm.org/D126162

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

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:335-336
+module_file = get_module_filename(module_path, module_name)
+if not os.path.isfile(module_file):
+  return ''
+with io.open(module_file, 'r') as f:

aaron.ballman wrote:
> Do we have to check this or can we rely on open failing because it's not a 
> file? (It tripped my psychic TOCTOU sensor.)
If we don't check here, then it throws an exception when attempting to open the 
file.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:352
+  stmt = code[stmt_start_pos+1:stmt_end_pos]
+  matches = re.search('registerCheck<([^>:]*)>\(\s*"([^"]*)"\s*\)', stmt)
+  if matches and matches[2] == full_check_name:

aaron.ballman wrote:
> It's a bit early for me to fully grok regex, but: is this going to handle 
> line continuations/newlines okay? I don't know if those show up in cases that 
> matter right now, but I wanted to make sure it was being thought about.
`\s` matches any single whitespace character and `\s*` matches zero or more 
whitespace characters.  I could sprinkle some more in between tokens, but this 
catches the existing code correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126134

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

[PATCH] D126134: [clang-tidy] Improve add_new_check.py to recognize more checks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added reviewers: aaron.ballman, njames93, alexfh.
LegalizeAdulthood added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
LegalizeAdulthood requested review of this revision.

When looking for whether or not a check provides fixits, the script
examines the implementation of the check.  Some checks are not
implemented in source files that correspond one-to-one with the check
name, e.g. cert-dcl21-cpp.  So if we can't find the check implementation
directly from the check name, open up the corresponding module file and
look for the class name that is registered with the check.  Then consult
the file corresponding to the class name.

Some checks are derived from a base class that implements fixits.  So if
we can't find fixits in the implementation file for a check, scrape out
the name of it's base class.  If it's not ClangTidyCheck, then consult
the base class implementation to look for fixit support.

Fixes #55630


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126134

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  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
@@ -37,19 +37,19 @@
`altera-struct-pack-align `_, "Yes"
`altera-unroll-loops `_,
`android-cloexec-accept `_, "Yes"
-   `android-cloexec-accept4 `_,
+   `android-cloexec-accept4 `_, "Yes"
`android-cloexec-creat `_, "Yes"
`android-cloexec-dup `_, "Yes"
-   `android-cloexec-epoll-create `_,
-   `android-cloexec-epoll-create1 `_,
-   `android-cloexec-fopen `_,
-   `android-cloexec-inotify-init `_,
-   `android-cloexec-inotify-init1 `_,
-   `android-cloexec-memfd-create `_,
-   `android-cloexec-open `_,
+   `android-cloexec-epoll-create `_, "Yes"
+   `android-cloexec-epoll-create1 `_, "Yes"
+   `android-cloexec-fopen `_, "Yes"
+   `android-cloexec-inotify-init `_, "Yes"
+   `android-cloexec-inotify-init1 `_, "Yes"
+   `android-cloexec-memfd-create `_, "Yes"
+   `android-cloexec-open `_, "Yes"
`android-cloexec-pipe `_, "Yes"
-   `android-cloexec-pipe2 `_,
-   `android-cloexec-socket `_,
+   `android-cloexec-pipe2 `_, "Yes"
+   `android-cloexec-socket `_, "Yes"
`android-comparison-in-temp-failure-retry `_,
`boost-use-to-string `_, "Yes"
`bugprone-argument-comment `_, "Yes"
@@ -105,7 +105,7 @@
`bugprone-terminating-continue `_, "Yes"
`bugprone-throw-keyword-missing `_,
`bugprone-too-small-loop-variable `_,
-   `bugprone-unchecked-optional-access `_, "Yes"
+   `bugprone-unchecked-optional-access `_,
`bugprone-undefined-memory-manipulation `_,
`bugprone-undelegated-constructor `_,
`bugprone-unhandled-exception-at-new `_,
Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -158,12 +158,17 @@
'namespace': namespace})
 
 
-# Modifies the module to include the new check.
-def adapt_module(module_path, module, check_name, check_name_camel):
+# Returns the source filename that implements the module.
+def get_module_filename(module_path, module):
   modulecpp = list(filter(
   lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
   os.listdir(module_path)))[0]
-  filename = os.path.join(module_path, modulecpp)
+  return os.path.join(module_path, modulecpp)
+
+
+# Modifies the module to include the new check.
+def adapt_module(module_path, module, check_name, check_name_camel):
+  filename = get_module_filename(module_path, module)
   with io.open(filename, 'r', encoding='utf8') as f:
 lines = f.readlines()
 
@@ -320,24 +325,100 @@
  os.listdir(docs_dir)))
   doc_files.sort()
 
+  # We couldn't find the source file from the check name, so try to find the
+  # class name that corresponds to the check in the module file.
+  def filename_from_module(module_name, check_name):
+module_path = os.path.join(clang_tidy_path, module_name)
+if not os.path.isdir(module_path):
+  return ''
+module_file = get_module_filename(module_path, module_name)
+if not os.path.isfile(module_file):
+  return ''
+with io.open(module_file, 'r') as f:
+  code = f.read()
+  full_check_name = module_name + '-' + check_name
+  name_pos = code.find('"' + full_check_name + '"')
+  if name_pos == -1:
+return ''
+  stmt_end_pos = code.find(';', name_pos)
+  if stmt_end_pos == -1:
+return ''
+  stmt_start_pos = code.rfind(';', 0, name_pos)
+  if stmt_start_pos == -1:
+stmt_start_pos = code.rfind('{', 0, name_pos)
+  if stmt_start_pos == -1:
+return ''
+  s

[PATCH] D125771: [clang-tidy] Add a useful note about -std=c++11-or-later

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/check_clang_tidy.py:31
+  -std=c++(98|11|14|17|20)[-or-later]:
+This flag will cause multiple runs within the same check_clang_tidy
+execution. Make sure you don't have shared state across these runs.

It's only the `-or-later` variants that cause multiple runs, so we should 
explain this more explicitly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125771

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

[PATCH] D117460: [clang-tidy][NFC] Reduce map lookups in IncludeSorter

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.
Herald added a project: All.

Are we waiting on some sort of benchmark before accepting/rejecting this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117460

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

[PATCH] D117409: [clang-tidy] Move IncludeInserter into ClangTidyContext

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood added a comment.
This revision is now accepted and ready to land.
Herald added a project: All.

If the only thing holding this up is my comments about following the Law of 
Demeter, I don't think that should block submitting this change.

I don't feel strongly about it enough to block submission and the style guide 
doesn't say anything about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117409

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

[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood 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/D125209/new/

https://reviews.llvm.org/D125209

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

[PATCH] D125769: [clang-tidy] Introduce the CheckHeaderFile option to modernize-deprecated-headers

[Richard via Phabricator via cfe-commits]

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

LGTM
Address one nit and ship.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125769

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

[PATCH] D125769: [clang-tidy] Introduce the CheckHeaderFile option to modernize-deprecated-headers

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:180
+  the ``CheckHeaderFile=true`` option if header files of the project only
+  included by c++ source files.
 

Capitalize to `C++`



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp:2-4
 // Copy the 'mylib.h' to a directory under the build directory. This is
 // required, since the relative order of the emitted diagnostics depends on the
 // absolute file paths which is sorted by clang-tidy prior emitting.

steakhal wrote:
> LegalizeAdulthood wrote:
> > IMO, all of this hackery is simply ducking the issue, which is that 
> > `check_clang_tidy.py` doesn't have proper support for validating fixits 
> > applied to header files.
> > 
> > See https://reviews.llvm.org/D17482
> I agree that this is nasty, but this was the only way I found to still test 
> the feature I'm about to introduce.
Agreed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125769

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

[PATCH] D125771: [clang-tidy] Add a useful note about -std=c++11-or-later

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/check_clang_tidy.py:22
 [-check-suffixes=] \
+[-std=(c++11-or-later|c++17)] \
\

These aren't the only two valid options for this argument  Valid values are:


  - `c++98-or-later`
  - `c++11-or-later`
  - `c++14-or-later`
  - `c++17-or-later`
  - `c++20-or-later`

and of course all the specific standard versions:

  - `c++98`
  - `c++11`
  - `c++14`
  - `c++17`
  - `c++20`




Comment at: clang-tools-extra/test/clang-tidy/check_clang_tidy.py:30
+Notes:
+  -std=c++11-or-later:
+This flag will cause multiple runs within the same check_clang_tidy

Probably need to explain the `-or-later` argument option more generally here 
and not just for C++11


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125771

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

[PATCH] D125769: [clang-tidy] Introduce the CheckHeaderFile option to modernize-deprecated-headers

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h:63
   std::vector IncludesToBeProcessed;
+  bool WarnIntoHeaders;
 };

whisperity wrote:
> LegalizeAdulthood wrote:
> > 1) How is this different from the clang-tidy option that specifies whether 
> > or not fixits are applied to header files?
> > 
> >   As an owner of a code base, I would know which header files are included 
> > from C source files and I would set my header-file regex (honestly, not a 
> > fan of a regex for that option; I'd prefer white/black lists, but that's 
> > another discussion) to exclude header files that are known to be included 
> > in C source files.
> > 
> > 2) Assuming that the header-file regex is somehow insufficient to cover 
> > this scenario, I like the functionality but the name of this option feels 
> > "off".  (Naming things is hard.)  Elsewhere we have options that say 
> > `HeaderFile` not `Headers` and `Into` just doesn't sound like the way 
> > normal conversation would state the situation.  Something like 
> > `CheckHeaderFile` would be more consistent with existing options.
> I do not know the answer to question //1//, but as an owner of a code-base, I 
> would not want to know or deal with keeping additional lists (be it 
> file-by-file, glob expressions, or regex...) specific to what headers are 
> includable from C and what aren't. Especially considering that every C header 
> could be included from C++. (Note how LLVM uses `.h` instead of `.hpp` for 
> its headers, even though >90% of our headers would never compile in C 
> mode...) If a check is misbehaving for my codebase, I'd just simply disable 
> that check (1 line of code) and go on with my life, instead of creating a 
> curated list, that is //at least// 2 LoC in a config file, //and// has to be 
> maintained down the line...
> 
> Moreover, the `-header-filter` regex and `-system-headers` are Tidy-level 
> global flags. This means that I would either have to let go of EVERY 
> potential diagnostic that might be placed in headers, or tinker with my 
> environment to run multiple instances of Clang-Tidy with different 
> configurations.
Those are all good points.  One of the weakest aspects of `clang-tidy` is the 
selection of files that should be analyzed and/or fixed.  Otherwise, we 
wouldn't need `clang-tidy/tool/run-clang-tidy.py` at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125769

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

[PATCH] D125771: [clang-tidy] Add a useful note about -std=c++11-or-later

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D125771#3525252 , @njames93 wrote:

> Tbh the whole testing infrastructure for clang-tidy is a mess. When I have 
> wanted to verify fixes and diagnostics for header files, I find manually 
> invoking clang-tidy is better than using the check_clang_tidy script.
> I don't have the time right now, but a verify mode like with clang is 
> something that we should move to in the future.

I had a change , literally years ago at this 
point, that provided support for verifying header files, but it got bogged down 
in review nits and never landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125771

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

[PATCH] D121372: [clang-tidy][docs][NFC] Update URL and docs of PostfixOperatorCheck

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:116
`bugprone-virtual-near-miss `_, "Yes"
-   `cert-dcl21-cpp `_,
+   `cert-dcl21-cpp `_, "Yes"
`cert-dcl50-cpp `_,

LegalizeAdulthood wrote:
> The problem with making this change manually in the `list.rst` is that the 
> next time someone runs `add_new_check.py`, the change will get lost again 
> unless someone reviews the diff carefully and reverts the unwanted change.  
> These manual reversions continue to accumulate and the chances for error 
> increase with each run of `add_new_check.py`.
> 
> This is additionally complicated by the fact that the check name doesn't 
> correspond algorithmically to the source file that implements the check, 
> which is how `add_new_check.py` figures out whether or not a check implements 
> fixits to add `"Yes"` for this column in the generated `list.rst`.
> 
> Here are some options I can see (there may be others, discussion welcome):
> 
> 
>   - Make the "official" check name `cert-postfix-operator` and add 
> `cert-dcl121-cpp` an alias?  This would enable the logic in 
> `add_new_check.py` to correctly match up the source file for the check to the 
> check name.
>   - Rename the source file to `Dcl121CppCheck.cpp`, but I don't think that's 
> as self-documenting as the existing file name.
>   - Have `add_new_check.py` consult the `xxxModule.cpp` files and scrape out 
> matching check names to check class names and use the class name to scrape 
> source files, possibly only when the corresponding source file that matches 
> the check name doesn't exist.
> 
> We should also add comments to `list.rst` to indicate that the file is 
> processed by a script.
Also, sorry for bringing this up much later.. I noticed it because I'm testing 
some other changes to `add_new_check.py` and it was reverting the change made 
here manually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121372

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

[PATCH] D121372: [clang-tidy][docs][NFC] Update URL and docs of PostfixOperatorCheck

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:116
`bugprone-virtual-near-miss `_, "Yes"
-   `cert-dcl21-cpp `_,
+   `cert-dcl21-cpp `_, "Yes"
`cert-dcl50-cpp `_,

The problem with making this change manually in the `list.rst` is that the next 
time someone runs `add_new_check.py`, the change will get lost again unless 
someone reviews the diff carefully and reverts the unwanted change.  These 
manual reversions continue to accumulate and the chances for error increase 
with each run of `add_new_check.py`.

This is additionally complicated by the fact that the check name doesn't 
correspond algorithmically to the source file that implements the check, which 
is how `add_new_check.py` figures out whether or not a check implements fixits 
to add `"Yes"` for this column in the generated `list.rst`.

Here are some options I can see (there may be others, discussion welcome):


  - Make the "official" check name `cert-postfix-operator` and add 
`cert-dcl121-cpp` an alias?  This would enable the logic in `add_new_check.py` 
to correctly match up the source file for the check to the check name.
  - Rename the source file to `Dcl121CppCheck.cpp`, but I don't think that's as 
self-documenting as the existing file name.
  - Have `add_new_check.py` consult the `xxxModule.cpp` files and scrape out 
matching check names to check class names and use the class name to scrape 
source files, possibly only when the corresponding source file that matches the 
check name doesn't exist.

We should also add comments to `list.rst` to indicate that the file is 
processed by a script.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121372

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

[PATCH] D124806: [clang-tidy] add support for Demorgan conversions to readability-simplify-bool-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D124806#3523228 , @njames93 wrote:

> In D124806#3523109 , 
> @LegalizeAdulthood wrote:
>
>> LGTM!  Thanks for taking my idea and implementing it much better than what I 
>> had `:)`.
>
> Cheers.
> Can I ask why you feel that this should be behind an option?

When I've brought it up in manual code reviews before, there were some 
objections.
That's why I thought it was worth adding an option to disable it.  Many people 
aren't
even familiar with DeMorgan's Theorem/Law and might be afraid that the 
simplification
changes semantics, although the underlying github issue shows that this is not 
the case.

> Also do you think there is merit in handling the case `!(A && B)` -> `!A || 
> !B` obviously that could be behind an option.

The reason I proposed the simplification `!(!A && B)` -> `A || !B` is because 
the former employs
"double negatives" and provides a stumbling block for readability.  All 
readability "checks"
are somewhat influenced by opinion and style to a certain extent, but I think 
the avoid
"double negative" idea is widely accepted.

In the case of `!(A && B)` -> `!A || !B`, no double negative is involved and 
one could argue
that it went from one negative to two, which is why I didn't include that in my 
original
proposal.  The "goodness" of it just doesn't seem universal and personally if 
the check
grew to enable that case, I'd want an option to disable that behavior `:)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124806

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

[PATCH] D125770: [clang-tidy] modernize-deprecated-headers should ignore system headers

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood 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/D125770/new/

https://reviews.llvm.org/D125770

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

[PATCH] D124650: [clang-tidy] Simplify boolean expressions by DeMorgan's theorem

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.

Discarding in favor of Nathan James's improved implementation.


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

https://reviews.llvm.org/D124650

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

[PATCH] D124806: [clang-tidy] add support for Demorgan conversions to readability-simplify-bool-expr

[Richard via Phabricator via cfe-commits]

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

LGTM!  Thanks for taking my idea and implementing it much better than what I 
had `:)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124806

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

[PATCH] D125770: [clang-tidy] modernize-deprecated-headers should ignore system headers

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp:37
 
-#include  // FIXME: We should have no warning into system 
headers.
-// CHECK-MESSAGES-WARN-INTO-HEADERS: mysystemlib.h:1:10: warning: inclusion of 
deprecated C++ header 'assert.h'; consider using 'cassert' instead 
[modernize-deprecated-headers]
+#include  // no-warning: Don't warn into system headers.
 

steakhal wrote:
> LegalizeAdulthood wrote:
> > steakhal wrote:
> > > LegalizeAdulthood wrote:
> > > > Where is this file?  I can't find it in my tree.
> > > > 
> > > > ```
> > > > D:\legalize\llvm\llvm-project\clang-tools-extra
> > > > > dir/s/b mysystemlib.h
> > > > File Not Found
> > > > ```
> > > > 
> > > > Does this patch depend on some other unsubmitted patch?
> > > D125209 should have included this file at 
> > > `clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h`.
> > OK, so it depends on D125209  why not just include these small changes 
> > in that?
> > 
> > Supposedly there is some way in phabricator to make one review dependent on 
> > another, but instead of being an expert in phabricator this change seems 
> > small enough to just fold into the other review, but perhaps opinions 
> > differ on that.
> I set the 'parent revision', so the dependencies are present under the 
> "Stack" tab. Sorry if I confused you.
> I still think it's a logically separate issue, which should be addressed in a 
> separate commit. That change is already quite confusing. I already had to 
> ever it once, thus I want to keep the cognitive complexity as low as possible 
> there. Same applies here.
> 
> Do you insist on merging these two?
Nope, no insistence on my part.  Just asking the question, which you've 
answered.

I never even noticed the "stack" tab in phabricator before.

Honestly, phabricator is not my favorite code reviewing tool, but it's what 
clang/llvm has chosen, so I live with it `:)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125770

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

[PATCH] D125885: [clang-tidy] bugprone-argument-comment: Ignore calls to user-defined literals

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood accepted this revision.
LegalizeAdulthood 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/D125885/new/

https://reviews.llvm.org/D125885

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