[PATCH] D101554: [clangd] Add support for the `defaultLibrary` semantic token modifier

[David Goldman via Phabricator via cfe-commits]

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