Page MenuHomePhabricator

[clangd] Added highlighting for structured bindings.
ClosedPublic

Authored by jvikstrom on Aug 26 2019, 6:21 AM.

Details

Summary

Structured bindings are in a BindingDecl. The decl the declRefExpr points to are the BindingDecls. So this adds an additional if statement in the addToken function to highlight them.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Aug 26 2019, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 6:21 AM
hokein added inline comments.Aug 26 2019, 7:56 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
236 ↗(On Diff #217130)

nit: could you spell out the type? it is not straight forward to infer from the method name.

238 ↗(On Diff #217130)

nit: we can group the above two ifs into one, like if (const auto* D = dyn_cast_or_null<...>(...getReferencedDecl).

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
437 ↗(On Diff #217130)

can we add a non-primitive field and test it?

442 ↗(On Diff #217130)

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?

447 ↗(On Diff #217130)

The above 4 lines seems not relevant to this patch, I think?

jvikstrom updated this revision to Diff 217652.Aug 28 2019, 7:43 AM
jvikstrom marked 6 inline comments as done.

Abandoned trying to highlight the same as the bound decl.

Reasoning: If you have a reference to a parameter we are not trying to highlight the reference as a parameter, the reference is highlighted as a variable. Don't really think bindings should be different.

I don't have a strong preference on how to highlight BindingDecl, but I think highlighting them as Variable is better than no highlighting them at all, so LGTM from myside. @ilya-biryukov thoughts?

again, please update the patch description in phabricator and your git commit message.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
443 ↗(On Diff #217652)

this is not related to the patch, but the highlighting behavior for auto here is weird, some of them are highlighted while some of them are not.

jvikstrom edited the summary of this revision. (Show Details)Aug 30 2019, 2:55 AM
jvikstrom marked an inline comment as done.Aug 30 2019, 2:58 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
443 ↗(On Diff #217652)

The reason for that is broken down here: https://reviews.llvm.org/D66516#inline-599982

Basically we don't traverse the type hierarchy when trying to add highlightings for auto so if we have a pointer/array type it will not find the actual type and just silently not add any highlightings.

I don't have a strong preference on how to highlight BindingDecl, but I think highlighting them as Variable is better than no highlighting them at all, so LGTM from myside. @ilya-biryukov thoughts?

Also think highlighting them as variables is the right thing.
e.g. if I do something like:

void foo(std::pair<int, int> p) {
  auto [f,s] = p;
  return f + s;
}

this is roughly equivalent to:

void foo(std::pair<int, int> p) {
  auto f = p.first;
  auto s = p.second;
  return f + s;
}

Therefore, it should also be highlighted in the same way.

hokein accepted this revision.Aug 30 2019, 4:23 AM
hokein added inline comments.
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
443 ↗(On Diff #217652)

thanks for the explanation.

This revision is now accepted and ready to land.Aug 30 2019, 4:23 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 7:06 AM