This is an archive of the discontinued LLVM Phabricator instance.

[clangd] A code tweak to expand a macro
ClosedPublic

Authored by ilya-biryukov on May 8 2019, 7:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.May 8 2019, 7:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 7:28 AM

This actually works, but still far from landing.
Notable problems:

  • this adds a dependency on TokenBuffer, so we need to land it first.
  • this change is too big, planning to split into multiple changes: (1) collecting tokens when building the AST for clangd, (2) add helpers into TokenBuffer
  • it currently tries to replace any "mapping", not just preprocessor expansions. E.g. will attempt to replace #define FOO ... with an empty string.
  • needs more tests.

Will get back to it after TokenBuffer lands.

ilya-biryukov planned changes to this revision.May 8 2019, 7:33 AM
  • Move logically separate parts to other changes
  • Rebase
  • Update some comments
sammccall accepted this revision.Jul 1 2019, 5:22 AM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
57 ↗(On Diff #206388)

it's pretty weird for a function whose return type is spelled "Token*" to use Spelled.end() rather than nullptr as a sentinel

125 ↗(On Diff #206388)

include macro name?

clang-tools-extra/clangd/unittests/TweakTests.cpp
288 ↗(On Diff #206388)

Can you verify we don't trigger here? FOO[[ ]]BAR

The zero-width range in FOO^ BAR is indeed interpreted as pointing at FOO by SelectionTree, but that's a whitespace-sensitive heuristic.

Given

int x(int);
#define B x
int y = B^(42);

The ^ points at the (. (Maybe we should lift this logic into Tweak::Selection)

This revision is now accepted and ready to land.Jul 1 2019, 5:22 AM
ilya-biryukov marked 5 inline comments as done.
  • Replace bsearch with partition_point.
  • Include macro name in the title.
  • Added a FIXME for empty selection case.
  • Return null when no token is found.
clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp
57 ↗(On Diff #206388)

Yeah, especially given that the function below uses nullptr as a sentinel...
Thanks for pointing this out!

clang-tools-extra/clangd/unittests/TweakTests.cpp
288 ↗(On Diff #206388)

It does trigger, unfortunately. There is no way to unbreak this, as we don't have a way to get the selection range in inputs of the tweak.
I've added a FIXME, will address with a follow-up.

Note that we don't use selection tree, as it's AST-based and we need token-level information for that tweak (that's pretty unique, I expect most tweaks are not like that)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 8:26 AM