sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
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:
> > 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
I think it'd be useful to do this for consistency, just VisitDecltypeTypeLoc
and VisitDeclaratorDecl need to be changed I think.
We'd want a version of isDefaultLibrary that works on Type. We're only going to
see canonical types so I think we need to:
- unwrap pointers (getPointeeOrArrayElementType)
- if BuiltinType -> return true
- for types: find the decl and dispatch to isDefaultLibrary(Decl)
to find the decl it'd be nice to use targetDecl() but also sufficient to handle
TagType + ObjCInterfaceType for now I think.
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
