This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Remove highlightings coming from non topLevelDecls from included files.
ClosedPublic

Authored by jvikstrom on Aug 12 2019, 5:20 AM.

Details

Summary

It is possible to write include code from other files so that the decls from there do not become topLevelDecls (For example by including methods for a class). These Decls are not filtered by topLevelDecls and therefore SemanticHighlighting must manually check that every SLoc belongs in the main file. Otherwise there can be highlightings appearing in places where they should not.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Aug 12 2019, 5:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2019, 5:21 AM
ilya-biryukov accepted this revision.Aug 12 2019, 5:25 AM

LGTM. Great example for the test case! It will definitely stay valid even if we fix all problems caused by RecursiveASTVisitor!

This revision is now accepted and ready to land.Aug 12 2019, 5:25 AM
hokein accepted this revision.Aug 12 2019, 5:30 AM
hokein added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
236 ↗(On Diff #214608)

nit: the ) is not match.

237 ↗(On Diff #214608)

nit: clangd has its own isInsideMainFile, please use it.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
57 ↗(On Diff #214608)

nit: maybe just std::vector<std::pair</*FileName*/llvm::StringRef, /*FileContent*/llvm::StringRef>>, it is clearer and would save you a long comment above.

This revision was automatically updated to reflect the committed changes.
jvikstrom marked 3 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2019, 6:00 AM