nridge added a comment. Thanks for the update! Looks pretty good, just a few minor comments from me. Will leave approval to Sam.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:470 std::vector<HighlightingToken> Tokens; + std::map<Range, std::set<HighlightingModifier>> ExtraModifiers; const HeuristicResolver *Resolver; ---------------- Looking at existing usage of associative containers in clangd code, `llvm::DenseMap` and `llvm::DenseSet` (which are hashtable-based) are more commonly used than `std::map` and `std::set`, though it may require some boilerplate to teach them to use new key types ([example](https://searchfox.org/llvm/rev/2556f58148836f0af3ad2c73fe54bbdf49f0295a/clang-tools-extra/clangd/index/dex/Token.h#117)) We could also consider making the value just a vector instead of a set (maybe even an `llvm::SmallVector<HighlightingModifier, 1>` given its anticipated usage), since even in the unlikely case that we get duplicates, `addModifier()` will handle them correctly ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:553 + if (auto *ProtoType = FD->getType()->getAs<FunctionProtoType>()) { + // Iterate over the declarations of the function parameters. + // If any of them are non-const reference paramters, add it as a ---------------- nit: declarations --> types ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:554 + // Iterate over the declarations of the function parameters. + // If any of them are non-const reference paramters, add it as a + // highlighting modifier to the corresponding expression ---------------- nit: parameters ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:560 + // Is this parameter passed by non-const reference? + if (T->isLValueReferenceType() && !T.getNonReferenceType().isConstQualified() && !T->isDependentType()) { + if (auto *Arg = Args[I]) { ---------------- I think we can relax the dependent type check here a bit, for example `std::vector<T>&` is a dependent type but it's definitely a mutable reference. (Feel free to leave that for later and just add a FIXME.) ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:566 + // e.g. highlight `a` in `a[i]` + if (auto *DR = dyn_cast<DeclRefExpr>(Arg)) { + Location = DR->getLocation(); ---------------- Maybe also add a `// FIXME: Handle dependent expression types` just so we don't forget ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:773 + )cpp", + /*struct $Class_decl[[S]] { + $Class_decl[[S]](int $Parameter_decl[[value]], const int& $Parameter_decl_readonly[[constRef]], ---------------- (test case needs uncommenting) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108320/new/ https://reviews.llvm.org/D108320 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
