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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 386047.
salman-javed-nz added a comment.

Revert to using simple string literals. I was being too clever for my own good 
with the Twine.
I think this should no longer cause ASan issues. WDYT?

This was meant to be a simple patch, so sorry for all the trouble.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -376,10 +376,10 @@
Context.getCurrentBuildDirectory(), false);
   StringRef Message =
   IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
+  ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+ "END' comment")
+  : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
+ "BEGIN' comment");
   Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
   return Error;
 }


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -376,10 +376,10 @@
Context.getCurrentBuildDirectory(), false);
   StringRef Message =
   IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
+  ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+ "END' comment")
+  : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
+ "BEGIN' comment");
   Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
   return Error;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D113472#3119292 , @jgorbe wrote:

> This change is causing asan errors when running the clang-tidy checks under 
> ASan, most likely because of the reason akuegel pointed out in his comment. 
> I'm going to revert the patch to unbreak HEAD until the problem can be fixed.

If you could do that, that would be appreciated. Thank you. I won't be able to 
get back to my desk to revert myself for at least half an hour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Jorge Gorbe Moya via Phabricator via cfe-commits]

jgorbe added a comment.

This change is causing asan errors when running the clang-tidy checks under 
ASan, most likely because of the reason akuegel pointed out in his comment. I'm 
going to revert the patch to unbreak HEAD until the problem can be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Adrian Kuegel via Phabricator via cfe-commits]

akuegel added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:381
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;

tooling::DiagnosticMessage expects a StringRef. If you pass it a str() from a 
local string, this can lead to bad pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the review.

I will think some more about your suggestion to look for `//`. Once I have 
something that works, I will be back for another review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG00769572025f: [clang-tidy] Fix lint warning in 
ClangTidyDiagnosticConsumer.cpp (NFC) (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

I suspect that ensuring that `NOLINT`s are within a comment could be a 
reasonably large code change. If it is to be robust with `/* multiline comments 
*/`  and other shenanigans, then I would probably lean on the compiler to tell 
us what is and isn't a comment, instead of the simple text search approach we 
currently have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

clang-tidy looking for `NOLINT` markers but not checking to see that the marker 
is within a comment, is long-standing behaviour at this point.
cpplint has the same behaviour, which explains why clang-tidy's implementation 
started off this way.

That's not to say that we have to keep this way forever. I would love for it to 
be improved.

However, discussing what the new behaviour should be and delivering it is 
beyond the scope of this patch, which is just trying to fix lint warnings in 
the current day framework.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz created this revision.
salman-javed-nz added a reviewer: carlosgalvezp.
salman-javed-nz added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
salman-javed-nz requested review of this revision.

Calling clang-tidy on ClangTidyDiagnosticConsumer.cpp gives a
"unmatched NOLINTBEGIN without a subsequent NOLINTEND" warning.

The "NOLINTBEGIN" and "NOLINTEND" string literals used in the
implementation of `createNolintError()` get mistaken for actual
NOLINTBEGIN/END comments used to suppress clang-tidy warnings.

Rewrite the string literals so that they can no longer be mistaken for
actual suppression comments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113472

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits