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

[salman-javed-nz]

Summary

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.

[carlosgalvezp]

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

[salman-javed-nz]

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 meantime with the current day NOLINT framework.

[salman-javed-nz]

I suspect that ensuring that NOLINTs are within a comment could be a reasonably large code change. If it is to be robust with /* multiline comments */ and \ characters 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.

[carlosgalvezp]

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.

[salman-javed-nz]

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.

[carlosgalvezp]

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.

[akuegel]

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

387

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

[jgorbe]

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.

[salman-javed-nz]

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.

[salman-javed-nz]

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.

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

 static ClangTidyError createNolintError(const ClangTidyContext &Context,
                                         const SourceManager &SM,
                                         SourceLocation Loc,
                                         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";
+          ? ("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;
 }
 
 static Optional<ClangTidyError> tallyNolintBegins(
     const ClangTidyContext &Context, const SourceManager &SM,
     StringRef CheckName, SmallVector<StringRef> Lines, SourceLocation LinesLoc,
     SmallVector<std::pair<SourceLocation, StringRef>> &NolintBegins) {
   // Keep a running total of how many NOLINT(BEGIN...END) blocks are active, as
   // well as the bracket expression (if any) that was used in the NOLINT
   // expression.
   size_t NolintIndex;
   StringRef NolintChecksStr;