dgoldman marked 5 inline comments as done.
dgoldman added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:433
+scopeModifier(const NamedDecl *D, const HighlightingsBuilder &HB) {
+ if (!D)
+ return llvm::None;
----------------
kadircet wrote:
> why the null check now?
removed and check for it when visit an obj method expr
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:436
+ if (HB.inDefaultLibrary(D->getLocation()))
+ return HighlightingModifier::DefaultLibrary;
const DeclContext *DC = D->getDeclContext();
----------------
sammccall wrote:
> 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.
Swapped over, currently don't check on deduced types but we could add our
isDefaultLibrary check there as well
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
