hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:236 + // binding if one exist. + if (const auto *BB = B->getBinding()) + if (const auto *RD = BB->getReferencedDeclOfCallee()) ---------------- nit: could you spell out the type? it is not straight forward to infer from the method name. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:238 + if (const auto *RD = BB->getReferencedDeclOfCallee()) + if (const auto *D = dyn_cast<NamedDecl>(RD)) { + addToken(Loc, D); ---------------- nit: we can group the above two ifs into one, like `if (const auto* D = dyn_cast_or_null<...>(...getReferencedDecl)`. ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:437 + struct $Class[[S]] { + $Primitive[[float]] $Field[[Member]]; + }; ---------------- can we add a non-primitive field and test it? ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:442 + $Primitive[[int]] $Variable[[A]][2] = {1,2}; + auto [$Variable[[B1]], $Variable[[B2]]] = $Variable[[A]]; + auto& [$Variable[[R1]], $Variable[[R2]]] = $Variable[[A]]; ---------------- From the code, I can't tell the `Variable` for the binding decl is from the underlying namedDecl or the fallback, could you add a comment clarifying it? ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:447 + $Field[[M1]] += 12.2; + $Variable[[B1]] += 2; + $Class[[S]] $Variable[[SArr]][2] = {$Class[[S]](), $Class[[S]]()}; ---------------- The above 4 lines seems not relevant to this patch, I think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66738/new/ https://reviews.llvm.org/D66738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits