sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:436
+ if (HB.inDefaultLibrary(D->getLocation()))
+ return HighlightingModifier::DefaultLibrary;
const DeclContext *DC = D->getDeclContext();
----------------
dgoldman wrote:
> 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?
> just let me know what you had in mind
I think this should follow `isStatic()` etc: just a boolean function that gets
called in the minimum number of places needed.
scopeModifier is slightly different pattern because there are a bunch of
mutually-exclusive modifiers. defaultLibrary is (mostly) independent of other
modifiers.
> if it's not useful for C++ maybe we could just make this configurable
No need really - sending more modifiers is cheap (one string at startup and one
bit in an integer we're sending anyway).
It's up to clients to style the ones they care about, and at least in VSCode
this can be customized per-lang I think. Important thing is that they don't
interact with other modifiers.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:465
+llvm::Optional<HighlightingModifier>
+scopeModifier(const Type *T, const HighlightingsBuilder &HB) {
if (!T)
----------------
dgoldman wrote:
> 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
>
>
Oops yes, in the other overload.
Please do remove this as Kadir suggests, it's noisy and makes inputs/outputs
less clear.
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
