This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
ClosedPublic

Authored by nridge on Feb 27 2020, 11:45 AM.

Details

Summary

Not handling this was a side-effect of being overly cautious when trying
to avoid reading files for which clangd doesn't have the source mapped.

Fixes https://github.com/clangd/clangd/issues/266

Diff Detail

Event Timeline

nridge created this revision.Feb 27 2020, 11:45 AM
sammccall added inline comments.Mar 4 2020, 2:49 PM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
353

why are we breaking the loop if we hit another file ID, rather than just not checking it?
I think a macro token can be expanded into another macro (with a non-main file ID) which in turn expands into the main-file - we'd want to reach the main file in that case.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
219–221

I do wonder a how hard it would be to approach this more directly: (conditionally )use some API for pulling code out of the SourceManager that *won't* silently do IO.
ContentCache::getRawBuffer() or something?

Up to you whether to look at this at all of course, this approach is better than what we have now.

clang-tools-extra/clangd/ParsedAST.cpp
312–313

this part of the comment seems stale

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
276

you may want a case with two levels of expansion

nridge updated this revision to Diff 248598.Mar 5 2020, 1:42 PM
nridge marked 4 inline comments as done.

Address review comments

nridge marked an inline comment as done.Mar 5 2020, 1:43 PM
nridge added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
219–221

I gave this a try in the updated patch.

sammccall accepted this revision.Mar 26 2020, 4:28 AM

Sorry for taking a while to come back to this again. A bit chaotic right now :-)

Looks good, some further simplification possible.

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

This seems more complicated than necessary - e.g. it replicates the "invalid" handling from SourceManager::getCharacterData, when we just bail out in this case. And keeping the raw char* interfaces seems unfortunate if we're going to the trouble of reimplementing most of this anyway.

What about implementing llvm::Optional<StringRef> getBuffer(FileID File, bool AllowIO)?
Where AllowIO determines whether you call getbuffer or getrawbuffer.

This is simpler than the current function, and the caller can be a bit more expressive/simpler like:

unsigned Offset = SM.getFileOffset(Loc);
StringRef RestOfLine = Data.substr(Offset).split('\n').first;
StringRef PrevLine = Data.substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
224

nit: I'd use AllowIO=true to avoid the negative sense, up to you.

This revision is now accepted and ready to land.Mar 26 2020, 4:28 AM
nridge updated this revision to Diff 253004.Mar 26 2020, 4:05 PM
nridge marked 2 inline comments as done.

Address review comments

nridge requested review of this revision.Mar 26 2020, 4:08 PM

Thanks for the suggested simplification. As the change is not completely trivial, could you have one more look before landing?

nridge updated this revision to Diff 253005.Mar 26 2020, 4:11 PM

Finish an incomplete variable extraction

Harbormaster completed remote builds in B50637: Diff 253005.
sammccall accepted this revision.Mar 27 2020, 7:53 AM

Thanks a lot!

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

nit: the peer function to reference is SourceManager::getBufferData

This revision is now accepted and ready to land.Mar 27 2020, 7:53 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
708–710

This looks like a clang-format artifact, there are several other below. Could these be removed from this patch

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
64

ditto

264–265

ditto

841–842

ditto

nridge marked an inline comment as done.Mar 28 2020, 10:12 AM
nridge added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
708–710

If you insist, I can move them to a separate patch. I don't want to leave it unmodified because the change will come back every time someone touches the file.

sammccall added inline comments.Mar 28 2020, 10:37 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
708–710

I don't personally care about this too much, but the changes are somewhat distracting in review, blame etc.

The policy we've mostly followed is to format changed lines only (git clang-format, turn format-on-save off) and leave misformatted code alone until it's next touched.

(Not sure if LLVM offers any guidance either way, but that's what Google does internally and IME it's been great)

nridge marked an inline comment as done.Mar 28 2020, 11:00 AM
nridge added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
708–710

The issue I have with that is that turning format-on-save off means you inevitably end up doing a lot of manual formatting as you write the code. With format-on-save, you can just write the tokens for a statement not caring about the formatting, do a save, and have the statement be formatted before you start writing the next one.

nridge marked an inline comment as done.Mar 28 2020, 11:06 AM
nridge added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
708–710

I guess what we really want is "format on save for modified lines only". I found a VSCode extension which does that, I'll give that a try.

njames93 added inline comments.Mar 28 2020, 5:37 PM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
708–710

What I do (apart from when i forget) is run git clang-format before creating the patch, Then just don't worry about formatting while I'm writing the code.

nridge updated this revision to Diff 253446.Mar 29 2020, 12:16 PM

Address last review comments

This revision was automatically updated to reflect the committed changes.