dgoldman marked an inline comment as done.
dgoldman 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();
----------------
sammccall wrote:
> 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.
Done, I think this is fine for a v1 - we don't see much auto/decltype use in
objc, can improve it later
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
