Page MenuHomePhabricator

[clangd] Ensure lambda init-capture gets semantic token
ClosedPublic

Authored by nridge on Sep 21 2021, 12:43 AM.

Details

Summary

Prior to this patch, CollectExtraHighlightings would incorrectly produce
a token for the init-capture's type which overlapped the name and
resulted in both being dropped.

Fixes https://github.com/clangd/clangd/issues/868

Diff Detail

Event Timeline

nridge created this revision.Sep 21 2021, 12:43 AM
nridge requested review of this revision.Sep 21 2021, 12:43 AM
nridge added inline comments.Sep 21 2021, 12:44 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
608

Note, I initially tried D->getTypeSpecStartLoc() != D->getTypeSpecEndLoc(), but it turns out they are equal both in the init-capture case and in the regular auto case, so that check cannot be used to discriminate between the two.

Naming of the patch is a little bit confusing. We're actually dropping the semantic highlighting for the type of lambdacaptures, which was showing up in the declarator names since there was no explicit type spelled in the source code. This turns on highlighting for the capture variables because we're left with a single token now.

Can you reword the description to reflect that?

clang-tools-extra/clangd/SemanticHighlighting.cpp
603–604

nit: while here do you mind turning this into an early exit as well? the nesting below seems a little distracting.

608

why not just check if D is implicit?

nridge updated this revision to Diff 374130.Sep 22 2021, 12:23 AM

address review comments

nridge retitled this revision from [clangd] Semantic highlighting for lambda init-capture to [clangd] Ensure lambda init-capture gets semantic token.Sep 22 2021, 12:24 AM
nridge edited the summary of this revision. (Show Details)
nridge marked an inline comment as done.

Naming of the patch is a little bit confusing. We're actually dropping the semantic highlighting for the type of lambdacaptures, which was showing up in the declarator names since there was no explicit type spelled in the source code. This turns on highlighting for the capture variables because we're left with a single token now.

Can you reword the description to reflect that?

Updated patch description. (Also, I forgot to link to https://github.com/clangd/clangd/issues/868 which contains additional context, sorry!)

clang-tools-extra/clangd/SemanticHighlighting.cpp
608

If you mean D->isImplicit(), that returns false for init-captures.

thanks, LGTM!

clang-tools-extra/clangd/SemanticHighlighting.cpp
603–604

sorry I was also talking about also turning if(auto K = ...) to

auto K = ...
if (!K)
  return true;
604

i think the comment might be more explicit about doing this to ensure we are not attributing the highlighting to some closeby token.

608

ah nvm, I was looking at the fielddecl implicitly introduced into the lambda, not the vardecl that was created with the capture (https://github.com/llvm/llvm-project/tree/main/clang/lib/Sema/SemaLambda.cpp#L1739).

kadircet accepted this revision.Sep 22 2021, 5:54 AM
This revision is now accepted and ready to land.Sep 22 2021, 5:54 AM
nridge updated this revision to Diff 374461.Sep 23 2021, 12:50 AM
nridge marked 3 inline comments as done.

address review comments

This revision was landed with ongoing or failed builds.Sep 23 2021, 12:51 AM
This revision was automatically updated to reflect the committed changes.