This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Ignore diags from builtin files
ClosedPublic

Authored by kadircet on Jul 17 2019, 6:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Jul 17 2019, 6:18 AM

Thanks for switching to SM everywhere, makes the code much more readable!

A rough proposal for testing this:

// test.cpp
// RUN: clang++ -DFOO=b ./test.cpp
int a = FOO;

This produces a note inside a <command line> buffer. We could probably have something similar pointing into a <builtin>

clang-tools-extra/clangd/Diagnostics.cpp
473 ↗(On Diff #210305)

There is also at least isWrittenInCommandLineFile and isWrittenInScratchSpace.
We should probably handle both of them here too.

What are we actually checking here? That we can later create a URI for this file? Is there a good way to check exactly that without breaking layering?

hokein added a subscriber: hokein.Jul 18 2019, 12:54 AM
hokein added inline comments.
clang-tools-extra/clangd/Diagnostics.cpp
473 ↗(On Diff #210305)

shall we put this into the isInsideMainFile? I think the location written in CommandLineFile/ScratchSpace is not inside main file logically.

ilya-biryukov added inline comments.Jul 18 2019, 1:26 AM
clang-tools-extra/clangd/Diagnostics.cpp
473 ↗(On Diff #210305)

Very good point.
@kadircet does isInsideMainFile returns true for locations from command line or scratch-pad?

kadircet updated this revision to Diff 210786.Jul 19 2019, 2:50 AM
  • Add tests.
  • Change the layer we ignore the diags:
    • Mark diags from headers as insidemainfile when we decide to surface them.
kadircet marked 2 inline comments as done.Jul 19 2019, 2:50 AM
kadircet added inline comments.
clang-tools-extra/clangd/Diagnostics.cpp
473 ↗(On Diff #210305)

yes it does

ilya-biryukov added inline comments.Jul 19 2019, 7:08 AM
clang-tools-extra/clangd/Diagnostics.cpp
601 ↗(On Diff #210786)

It's not clear why we these conditions should be checked somewhere else.
Could you explain in more detail?

kadircet updated this revision to Diff 210834.Jul 19 2019, 7:31 AM
  • Move deduplication logic back into the flushLastDiag as discussed offline.
ilya-biryukov added inline comments.Jul 19 2019, 7:37 AM
clang-tools-extra/clangd/Diagnostics.cpp
128 ↗(On Diff #210834)

NIT: inline StartPos, it has online a single usage now.

563 ↗(On Diff #210834)

We probably want to always set the value of this field to avoid accidentally reading the flag for the previous LastDiag

608 ↗(On Diff #210834)

NIT: maybe be more specific? coming from each particular header?

clang-tools-extra/clangd/Diagnostics.h
148 ↗(On Diff #210834)

NIT: add a comment that it was adjustDiagFromHeader that made the adjustment.

kadircet updated this revision to Diff 210838.Jul 19 2019, 7:45 AM
kadircet marked 5 inline comments as done.
  • Address comments
kadircet added inline comments.Jul 19 2019, 9:19 AM
clang-tools-extra/clangd/Diagnostics.cpp
563 ↗(On Diff #210834)

I don't follow, this is always set in line 475 at the beginning of HandleDiagnostic method.

ilya-biryukov added inline comments.Jul 22 2019, 4:11 AM
clang-tools-extra/clangd/Diagnostics.cpp
563 ↗(On Diff #210834)

I might be missing something, but the only assignment to LastDiagWasAdjusted seems to be here (apart from initialization at the declaration site)

kadircet updated this revision to Diff 212131.Jul 28 2019, 11:48 PM
kadircet marked 3 inline comments as done.
  • Always set LastDiagWasAdjusted.
clang-tools-extra/clangd/Diagnostics.cpp
563 ↗(On Diff #210834)

oopsy, fixed

ilya-biryukov accepted this revision.Jul 29 2019, 10:27 AM

LGTM. Many thanks for fixing this.

clang-tools-extra/clangd/Diagnostics.cpp
111 ↗(On Diff #212131)

NIT: add a small comment mentioning that returned value indicates if Diag was changed

475 ↗(On Diff #212131)

NIT: could use auto & here, the SourceManager is mentioned explicitly in the initializer anyway.

563 ↗(On Diff #212131)

NIT: I suggest moving the control-flow (&&) into a separate if statement.
adjustDiagFromHeader mutates its arguments and having complex expressions in the same statement makes it a little hard to notice that things might get mutated there.

But up to you.

clang-tools-extra/clangd/Diagnostics.h
148 ↗(On Diff #212131)

NIT: it's not adjustDiagFromHeader that actually sets the value, maybe rephrase to avoid confusion?
Something like

/// Set iff adjustDiagFromHeader resulted in changes to LastDiag.
This revision is now accepted and ready to land.Jul 29 2019, 10:27 AM
kadircet updated this revision to Diff 212307.Jul 30 2019, 3:21 AM
kadircet marked 3 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 3:26 AM