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

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:413
+  /// different heuristics may be used in the future (e.g. sysroot paths).
+  bool inDefaultLibrary(SourceLocation Loc) const {
+    if (!Loc.isValid())
----------------
kadircet wrote:
> you can make this a free function by accepting a decl instead. as sourcemanager is accessible via `D->getASTContext().getSourceManager()`. this way you don't need to modify signatures for `scopeModifier` overloads (and get rid of the duplicate in `CollectExtraHighlightings`).
TIL decls can reference the ASTContext. I can swap over to that if you'd like, the one advantage the current approach has is that we can easily add a caching layer into the Builder to say cache the results by FileID (but I'm not sure how useful/necessary such caching would be)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:436
+  if (HB.inDefaultLibrary(D->getLocation()))
+    return HighlightingModifier::DefaultLibrary;
   const DeclContext *DC = D->getDeclContext();
----------------
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?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:465
+llvm::Optional<HighlightingModifier>
+scopeModifier(const Type *T, const HighlightingsBuilder &HB) {
   if (!T)
----------------
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




================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:474
+    return scopeModifier(TD, HB);
+  if (auto *OT = dyn_cast<ObjCObjectType>(T))
+    return scopeModifier(OT->getInterface(), HB);
----------------
kadircet wrote:
> should this be put in a separate patch?
If you want I can move it over


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101554/new/

https://reviews.llvm.org/D101554