dgoldman 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())
----------------
kadircet wrote:
> 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`).
TIL decls can reference the ASTContext. I can swap over to that if you'd like,
the one advantage the current approach has is that we can easily add a caching
layer into the Builder to say cache the results by FileID (but I'm not sure how
useful/necessary such caching would be)
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:436
+ if (HB.inDefaultLibrary(D->getLocation()))
+ return HighlightingModifier::DefaultLibrary;
const DeclContext *DC = D->getDeclContext();
----------------
sammccall wrote:
> kadircet wrote:
> > 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.
> I think default-library isn't a scope modifier at all, it should be
> independent.
>
> e.g. std::string is defaultlLibrary | globalScope, while
> std::string::push_back() is defaultLibrary | classScope.
I can break it up if you'd like - just let me know what you had in mind. I can
change the scopeModifier function's name and then have it return a list of
modifiers or I can just add a new function which all existing callers of
scopeModifier will also need to call.
And what we're really trying to do here is color the system/SDK symbols
differently - this is definitely useful for iOS dev since Apple has a very
large SDK, but if it's not useful for C++ maybe we could just make this
configurable?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:465
+llvm::Optional<HighlightingModifier>
+scopeModifier(const Type *T, const HighlightingsBuilder &HB) {
if (!T)
----------------
sammccall wrote:
> I'm confused about the interface change - looks like the new parameter never
> actually used, just passed around
It's used (currently) as the HighlightingsBuilder is the one who checks for
default/systemness
================
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);
----------------
kadircet wrote:
> should this be put in a separate patch?
If you want I can move it over
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
