This is an archive of the discontinued LLVM Phabricator instance.

[clangd] A code action to qualify an unqualified name
Needs ReviewPublic

Authored by sammccall on Jan 11 2019, 10:12 AM.

Details

Reviewers
ilya-biryukov

Event Timeline

ilya-biryukov created this revision.Jan 11 2019, 10:12 AM
  • Add the code to actually instantiate a code action
  • Add some forgotten helpers

This is a somewhat simple action to illustrate the use of code action APIs.
Still missing tests and trying to figure out what information we want to expose in order to avoid walking over ASTs in each of the actions, so this is not final. Should be a good reference point for how the code of the action would look like, though.

Hi Ilya, I got here from reading your other patch (https://reviews.llvm.org/D56611). I'm wondering if we could make those range utility functions more understandable.

clangd/SourceCode.h
64

It seems to me the range is actually closed on both sides.
Do I get it right that the result is?

[begin of first token : end of last token]

Wouldn't some name like toWholeTokenRange be easier to understand?

81

I'd find it helpful to mention that the range is actually interpreted as closed-open (knowing which half is which).

  • Update after changes to parent revision
ilya-biryukov added inline comments.Jan 16 2019, 9:50 AM
clangd/SourceCode.h
64

The range is supposed to be half-open to allow representing points as empty range starting at the specified position.
This literally represents two byte offsets inside the same file.

My plan is to introduce a class to capture this abstraction, but let me see how the review goes.

81

Good point, will do.

  • Rebase after parent change
  • Update to reflect changes in parent revision
  • Remove the header file
  • Move to the monorepo
  • Move out the source code helpers (they're now in swap-if-branches)

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

No, this action is not about adding a type qualifier (like const or volatile), it adds a namespace qualifier, e.g.

using namespace std;

vector<int> foo; // --> std::vector<int> foo;

Moreover, it's just an example to illustrate how one could write a simple action like that in clangd.

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

No, this action is not about adding a type qualifier (like const or volatile), it adds a namespace qualifier, e.g.

using namespace std;

vector<int> foo; // --> std::vector<int> foo;

Moreover, it's just an example to illustrate how one could write a simple action like that in clangd.

I see, nvmd then :)

  • Update license header
sammccall commandeered this revision.Feb 6 2019, 6:34 PM
sammccall edited reviewers, added: ilya-biryukov; removed: sammccall.

(Grabbing this as discussed offline)

sammccall updated this revision to Diff 185690.Feb 6 2019, 6:39 PM

Update matching to use Inputs.ASTSelection.
Cover some more cases of names (I think?)

Handle under-qualified names as well as unqualified ones.
The main benefit of this is it's a step closer to extracting all references to
qualified/unqualified names from the selection.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 6:39 PM
hokein added a subscriber: hokein.Feb 7 2019, 1:43 AM

You'd need to rebase this patch, D57739 had some changes to the Tweak API.