[PATCH] D66738: [clangd] Added highlighting for structured bindings.

[Haojian Wu via Phabricator via cfe-commits]

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