This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement getBeginning for overloaded operators.
ClosedPublic

Authored by hokein on Sep 18 2019, 12:42 AM.

Event Timeline

hokein created this revision.Sep 18 2019, 12:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 12:42 AM
hokein marked an inline comment as done.Sep 18 2019, 12:45 AM
hokein added inline comments.
clang-tools-extra/clangd/SourceCode.cpp
279

the function name doesn't match what it does now, considering a new name for it, getBeginningOfIdentifierOrOverloadedOperator seems too verbose, maybe just getBeginning?

hokein updated this revision to Diff 221903.Sep 26 2019, 2:41 AM

Rewrite the getBeignningOfIdentifier function.

hokein updated this revision to Diff 221904.Sep 26 2019, 2:42 AM

Fix a typo.

Harbormaster completed remote builds in B38582: Diff 221904.
ilya-biryukov added inline comments.Sep 26 2019, 4:49 AM
clang-tools-extra/clangd/SourceCode.cpp
320–339

If we use IgnoreWhitespace = false, I think we can simplify to:

// Location in the middle of some token.
if (BeforeLoc == CurrentLoc)
  return CurrentLoc;

// Whitespace is not interesting.
if (CurrentTok.Kind == tok::whitespace) 
  return BeforeTok;
if (BeforeTok.Kind == tok::whitespace)
  return CurrentTok;

// The cursor is at token boundary, i.e. `Before^Current`.
// Prefer identifiers to other tokens.
if (CurrentTok.Kind == tok::raw_identifier)
  return CurrentLoc;
if (BeforeTok.Kind == tok::raw_identifier)
  return BeforeLoc;
// And overloaded operators to other tokens.
if (isOperator(CurrentTok.Kind))
  return CurrentLoc;
if (isOperator(BeforeTok.Kind))
  return BeforeLoc;
return CurrentLoc;

That would have the same semantics, but is arguably simpler. WDYT?

hokein updated this revision to Diff 221935.Sep 26 2019, 6:36 AM
hokein marked 2 inline comments as done.

Simplify the logic.

hokein updated this revision to Diff 221936.Sep 26 2019, 6:37 AM

Upload correct diff.

clang-tools-extra/clangd/SourceCode.cpp
320–339

Good point, that would simplify the logic.

Harbormaster completed remote builds in B38595: Diff 221936.

Mostly NITs, except the naming of the new TokenKind enum.
I think it's better to pick something that's not clashing with clang::tok::TokenKind, even if the enum is in a different namespace.

clang-tools-extra/clangd/SourceCode.cpp
242

TokenKind has the same name as tok::TokenKind. Could we use a different name here to avoid possible confusions?
E.g. TokenGroup or TokenFlavor.

258

NIT: add static for consistency with the rest of the function.

261

NIT: can this just be tok::NUM_TOKENS? same for other kinds in the same function

311

NIT: Could you please duplicate what case 2 is to avoid the need to go back to the comment at the top.

326

could be at the token boundary should be is at the token boundary, right?
Whenever it's not the case we'll bail out on BeforeTokBeginning == CurrentTokBeginning.

hokein updated this revision to Diff 222423.Sep 30 2019, 7:00 AM
hokein marked 8 inline comments as done.

address comments.

clang-tools-extra/clangd/SourceCode.cpp
242

renamed to TokenFlavor.

258

I think the static is redundant here, as the function is in the anonymous namespace, I removed the static on the function above.

326

yes, you are right. I was thinking of the single token case int ^foo;, which should be counted into the boundary case as we consider whitespace as a token kind.

ilya-biryukov accepted this revision.Sep 30 2019, 9:21 AM

Mostly LGTM

clang-tools-extra/clangd/SourceCode.cpp
258

LG, that's also fine.

I suggested 'static' because other functions in this file are using it (in addition to being in the anonymous namespace, I believe).

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
330

Do we test ++^^abc anywhere?

This revision is now accepted and ready to land.Sep 30 2019, 9:21 AM
ilya-biryukov added inline comments.Oct 1 2019, 12:05 AM
clang-tools-extra/clangd/SourceCode.cpp
279

getBeginning sounds too generic too me, would definitely avoid that. I think getBeginningOfIdentifier creates less confusion, albeit it definitely creates some.

No better alternative comes to mind, though. Maybe keep as is and update the comment to mention it also handles tokens for operators?

hokein updated this revision to Diff 222583.Oct 1 2019, 3:52 AM
hokein marked 2 inline comments as done.

add one more testcase.

hokein added inline comments.Oct 1 2019, 3:55 AM
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
330

no, but we have a similar test ++/**/^abc, added it.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 4:03 AM