kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:413
+ /// different heuristics may be used in the future (e.g. sysroot paths).
+ bool inDefaultLibrary(SourceLocation Loc) const {
+ if (!Loc.isValid())
----------------
you can make this a free function by accepting a decl instead. as sourcemanager
is accessible via `D->getASTContext().getSourceManager()`. this way you don't
need to modify signatures for `scopeModifier` overloads (and get rid of the
duplicate in `CollectExtraHighlightings`).
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:433
+scopeModifier(const NamedDecl *D, const HighlightingsBuilder &HB) {
+ if (!D)
+ return llvm::None;
----------------
why the null check now?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:436
+ if (HB.inDefaultLibrary(D->getLocation()))
+ return HighlightingModifier::DefaultLibrary;
const DeclContext *DC = D->getDeclContext();
----------------
I don't use semantic highlighting enough to judge whether it is more useful to
say `DefaultLibrary` than `{Class,Function}Scope`. (i.e. having the check here
vs at the end of the function as a fallback). Can you provide some rationale
about the decision (probably as comments in code)?
By looking at the testcase, I suppose you are trying to indicate
"overriden"/"extended" attribute of a symbol, which makes sense but would be
nice to know if there's more. I am just worried that this is very obj-c
specific and won't really generalize to c/c++. As most of the system headers
only provide platform specific structs (e.g. posix) and they are almost never
extended. So it might be confusing for user to see different colors on some
memberexprs.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:474
+ return scopeModifier(TD, HB);
+ if (auto *OT = dyn_cast<ObjCObjectType>(T))
+ return scopeModifier(OT->getInterface(), HB);
----------------
should this be put in a separate patch?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101554/new/
https://reviews.llvm.org/D101554
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
