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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
