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

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

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

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] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

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

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

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;

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

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.

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

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/

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

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

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

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

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

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,

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

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

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

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"?

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

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