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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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();
----------------
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:465
+llvm::Optional<HighlightingModifier>
+scopeModifier(const Type *T, const HighlightingsBuilder &HB) {
   if (!T)
----------------
I'm confused about the interface change - looks like the new parameter never actually used, just passed around


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:75
   GlobalScope,
+  DefaultLibrary,
 
----------------
This isn't a scope modifier so shouldn't be grouped with them


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101554