This is an archive of the discontinued LLVM Phabricator instance.

Clang Rename Tool
ClosedPublic

Authored by klimek on Jul 31 2014, 1:46 AM.

Details

Reviewers
klimek
mplant
Summary

This is the first patch for the clang rename tool. In order to reduce the amount of code in this patch, a fair amount of code has been removed from USRFinder.cpp and USRLocFinder.cpp, at the request of Manuel. This reduces the functionality of the tool to just renaming variables, and wont rename them in all contexts. The remaining code will be added when it is found to be appropriate (it does in fact exist; I have it in a tar in my home directory).

It builds fine, and I can run the program just fine on my home laptop as well as my work computer.

Diff Detail

Event Timeline

mplant updated this revision to Diff 12059.Jul 31 2014, 1:46 AM
mplant retitled this revision from to Clang Rename Tool.
mplant updated this object.
mplant edited the test plan for this revision. (Show Details)
mplant added a reviewer: klimek.
mplant added subscribers: Unknown Object (MLST), amshali.
klimek edited edge metadata.Jul 31 2014, 11:31 AM

First round focused on ClangRename.cpp. More to come :)

clang-rename/ClangRename.cpp
70–72

I usually find offsets better to use for tools (and humans are not going to enter lines and columns here anyway).

83

This is also already a feature of ClangTool / RefactoringTool if I'm not mistaken.

91

I'd try to not use getNameAsString to do matching against - can't we solve this in the type system?

101

Name this getAllConstructorUSRs?

114

I wonder whether we could do something similar for constructors (perhaps by changing the AST).

126–129

Needs a comment.

132

points

132–134

Renaming operators seems ... strange; I would expect to add that (if at all) somewhen much later.

170

I find this rather strange architecturally - why don't we just visit the whole thing twice?

174

s/mail/main/

234

Remove comment.

249–259

Any reason you're not using RefactoringTool (which already does basically everything that's in this file)?

mplant updated this revision to Diff 12080.Jul 31 2014, 1:09 PM
mplant edited edge metadata.

Responded to comments and such

clang-rename/ClangRename.cpp
70–72

I added the option to use an offset. And I would not be too sure about the second claim. No matter how obtuse an interface, someone will prefer it. After all, ed is bundled with every Ubuntu install ;)

83

You are correct. As explained later, we can't use RefactoringTool, unless you can think of a solution to the problem explained later.

91

Not in a way that also conveniently gives us the operator spelling.

101

I was thinking that there would be other get*EquivUSRs functions. The term "Equivalent USRs" makes more sense to me in a context where there are multiple of them. But that is not the case here, so I'll change it.

114

I'm not so sure. Constructors and destructors are pretty different; there can only be one destructor for example.

132–134

It is very strange, but it's there. The code for fixOperatorLocs however is not too complicated, and we already have some code for operator renaming I'm planning to place in USR(Loc)Finder later. So I'd rather not remove this just for the sake of the review.

234

But then how will anyone know what the '}' means?? (kidding)

249–259

We need to create a factory for RefactoringTool, which allocates and returns a new consumer per file. Unfortunately, we can't in the factory control where the allocated consumer is deleted - it's free'd by RefactoringTool! This means we can't have persistent state in our consumer without global or static variables - which would require us to violate http://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors. Since we need persistent state for the USR (see comment above), we are left with avoiding RefactoringTool

mplant added inline comments.Jul 31 2014, 1:10 PM
clang-rename/ClangRename.cpp
170

The reason is that the point may not be in the file we are currently looking at. If we were to attempt to find the USR, we would not find it. Thus we must save it when we initially find it.

this is part of the reason why we can't use refactoring tool - see below for more info on that.

klimek added inline comments.Jul 31 2014, 9:16 PM
clang-rename/ClangRename.cpp
67–76

Ok, I'm *very* opposed to having both interfaces. This just complicates the code. I stand by what I said - I think users will not enter this manually (because that doesn't really make sense), so we should only use the offset.

91

I thought there was a different way to get the operator name... I'll take a look.

132–134

"It's there" is not a good argument. Neither is "it's not too complicated".
A good argument for a feature needs to involve "it's useful". Note that the code will be maintained by other people, which is why we have pretty strict requirements on the code.

170

I'm not sure I understand:
If you're concerned about finding it when we want to look at multiple files, we have to make sure we look at the file that has the USR *first*, otherwise we might miss refactorings, right?
If you're only concerned about a single file, visiting it twice won't hurt, and be architecturally the right thing to do so we don't run into problems later.

249–259

"This means we can't have persistent state in our consumer without global or static variables"

Just have the state at function scope (for example, here, in a class) and pass a pointer to that state down through the factory into the consumer. The consumer doesn't need to delete things it references when it's destroyed (or I'm missing something).

Questions before (hopefully) substantial cleanup

clang-rename/ClangRename.cpp
67–76

It's your decision, but let me also add that this will make writing shell scripts and such that use the command line tool.
Imagine a shell script that took every "the symbol at x:y:z could not be found, did you mean <other symbol>" and renamed the file accordingly.

132–134

It is useful, in some instance. because "new-name" can be any string, you could use it to do some pretty cool stuff I'm sure.
Also I don't have any problem maintaining just this tool, although I imagine there's just one maintainer for clang-tools-extra. It's not like I have work during the school year after all.

170

Yes, we do have to make sure we're looking at the file that has the USR first. That is guaranteed actually, because we push it to the front of the FileNames list.

249–259

That solves one problem, but there's another:
What about the actual renaming part? I seem to recall a rewriter will become invalid when the RefactoringTool gets a new consumer. Does the factory need to create a new rewriter each time it creates a new consumer?

For what it's worth, I did try to use RefactoringTool; there's more examples on it. But for some reason during my experimentation I concluded that it was insufficient.

mplant updated this revision to Diff 12134.Aug 1 2014, 5:56 PM

Updated to use RefactoringTool

mplant added a comment.Aug 1 2014, 5:59 PM

Updated to use RefactoringTool - and removed support for file:line:column arguments. I need to update rename_test.py to fix this at some point.

klimek added inline comments.Aug 4 2014, 3:22 AM
clang-rename/ClangRename.cpp
160–189

As mentioned before, I think we want an extra run for this, so it can be called outside the RenamingASTConsumer. It seems completely unrelated to renaming.

202–207

What's the reason not to sink this into getLocationsOfUSR?

216–217

I'd pull that out of the loop and invert the if (PrintLocations) to only affect the errs() <<. Also, I'd probably use outs() unless this is for debugging, in which case we'd want to use the debugging facilities instead of just dumping to stderr...

263

I'd prefer this class (and everything it uses) to go into its own .h/.cpp file.
Perhaps call it RenamingAction.h/.cpp.

I'd still want to modularize it further:

  • One action to find the USRs (perhaps just export it from USRFinder.h)
  • (mid-term) An AST matcher matchUSRs(const std::vector<std::string>&) that matches when a node matches one of the given USRs; this is non-trival to implement, so I'm fine with having an extra AST consumer for now (the getLocationsOfUSRs seems like a good abstraction for now, but I'd want to add a FIXME)
  • most of the code in RenamingASTConsumer seems non-renaming related; see comments there.
275

Please add:
// FIXME: Use clang diagnostics.

284

You don't actually need this.
Rename CreateASTConsumer to createASTConsumer, and you'll be able to just say:
newFrontendActionFactory(&Action)
below.

clang-rename/USRFinder.cpp
42

I'd prefer giving this class a getResults() method over handing in a NamedDecl**.

102–126

It's amazing how most of these functions seem to be handling overloaded operators :(

210–213

Should we check that Start and End are in the same file?

clang-rename/USRFinder.h
35

Nit: add newline below.

clang-rename/USRLocFinder.cpp
41–49

I think we either want to test this (which means it will not need a "This is correct as of" comment), or we don't want to rely on it.

test/clang-rename/VarTest.cpp
4

I'm honestly not sure I like this better than:
// CHECK: int bar;

mplant updated this revision to Diff 12180.Aug 4 2014, 2:41 PM

Various refactorings.

mplant added a comment.Aug 4 2014, 2:41 PM

comments

clang-rename/ClangRename.cpp
160–189

I agree, but I cannot refactor this to such a degree in such a short amount of time.

202–207

Because changing the location is only useful for renaming. If someone wants to use getLocationsOfUSR for anything else, they probably want a different location.

216–217

That would best be left when more of the code is refactored sensibly and renamed locations are easy to extract. At least by printing to stderr the data can be sensibly removed from the output.

263

I have done bits of the first parts, but future goals will have to wait until I move back to school

284

that is the weirdest interface in the world.

clang-rename/USRFinder.cpp
42

Yeah I changed this. Really I think that this could be done a lot better with a few static methods, but I really don't have enough time for that within the next week. It'll have to wait for later in the year.

102–126

It was decided (not by me) that out of the two things left to possible add before up-streaming - macros and overloaded operators - the latter was to be completed.

Also, that's not entirely the case - most of the code doesn't, but the code that handles common cases (like operators and variables) are places where operator overloading needs to be accounted for.

210–213

I guess...

clang-rename/USRLocFinder.cpp
41–49

We don't rely on it. I'll remove the comment

test/clang-rename/VarTest.cpp
4

This allows us to automatically find the location of the symbol. That way, it checks USRFinder's accuracy as well.

4

this lets the tool automatically find locations. I assure you, it is much easier.

klimek added inline comments.Aug 5 2014, 12:44 AM
clang-rename/ClangRename.cpp
160–189

If your time does not permit finishing this through, I'd say we stop the review now and start it once somebody is willing to pick up the patch and see it through to the end.

test/clang-rename/VarTest.cpp
4

But the tests are much harder to read. I'd actually say we want two kinds of tests: one for the USR finder, and one for the rename. The USR finder one might even be a unit test.

mplant updated this revision to Diff 12257.Aug 6 2014, 3:22 PM

Modularizes clang-rename further, now with separate actions to find the USR and perform renaming.

klimek added a comment.Aug 7 2014, 2:50 AM

This starts to look really good. Thanks for bearing with me :)

I'd really like to remove the conditional operator renaming for the first version - I think it'll be much easier to follow the code that way (and I don't think it'd be too much work).

For testing, I think there are multiple layers:

  1. test that the location finding machinery works - for that I'd actually use unit tests; you can take a look at LocationVerifier in unittest/AST/MatchVerifier
  2. test that the renaming works; for that, I think FileChecks are perfect, as you can just OldType t; // CHECK: NewType t;
clang-rename/ClangRename.cpp
70–71

Can you explain what exactly didn't work? Perhaps give me the compile error? But it's no biggie - I can also get rid of this once it's checked in - the template error messages are sometimes hard to get through :)

121–134

Nice, this looks much better structured now. Thanks!

I'll get rid of the operator renaming code and take a look at unit tests.

clang-rename/ClangRename.cpp
70–71

It spat out a virtual method not implemented error. I've grepped through the code, and I couldn't find one interface with a virtual method createASTConsumer. Did you mean some method with a different name?

mplant updated this revision to Diff 12320.Aug 8 2014, 4:56 PM

Added renaming unittest and converted VarTest.cpp to use FileCheck. The added unittest builds and passes fine.

klimek added inline comments.Aug 11 2014, 2:30 AM
clang-rename/ClangRename.cpp
17–43

Are all of these still needed?

clang-rename/RenamingAction.cpp
35–42

Generally, we shouldn't rely on global variables to communicate between classes. The solution is to put all flags into the main file, and pass the information into the relevant classes via parameters.

clang-rename/USRFinder.cpp
57–58

You removed the operator stuff in the other file, but here it's still in. Is that intentional? Will tests break if we remove it? :)

clang-rename/USRFindingAction.h
33

I don't think "Prev" is a good name part here... (I assume this is left from when this was strictly for renaming).

Also, I'd prefer to have private variables and getters here (otherwise it's unclear what is to be set from where).

test/clang-rename/VarTest.cpp
4–9

Thanks! This is much easier for me to read :)

unittests/clang-rename/USRLocFindingTest.cpp
62–67

I'm not 100% sure I understand what the branch is for here (and the function is lacking comments ;)
It looks like it's basically saying "those offsets should yield the same USR", right?
I think that's a good test idea (as it keeps the USR as an implementation detail, which is nice), but why not use a different data structure here, like:
vector<vector<unsigned>> offsets;
where offsets[group] is a list of offsets that should evaluate to the same USR?

75

You can use raw strings to make this look better...

112

Nit: remove redundant empty line.

mplant updated this revision to Diff 12412.Aug 12 2014, 12:06 PM

Updates, fixes, and misc.

clang-rename/ClangRename.cpp
17–43

Not entirely sure. The only real way to determine is to remove one at a time, or manually go through and determine if each is actually used.
Isn't there a clang tool for this? remove-unused-headers or something?

clang-rename/RenamingAction.cpp
35–42

Right-o, will fix. Thought it was a little dubious but decided to do it anyway.

clang-rename/USRFinder.cpp
57–58

Not intentional; I'll remove it.

clang-rename/USRFindingAction.h
33

Hmmm, sure. I renamed PrevName to SpellingName, I think that's better. Also added the setters and getters.

test/clang-rename/VarTest.cpp
4–9

No problem, but boy was that offset hard to find :P

unittests/clang-rename/USRLocFindingTest.cpp
62–67

You're correct on the intent. The idea is that we don't want to rely on USRs being any particular value so long as that the invariants described in rename_test.py are held. I'm not sure why I didn't choose a vector of vectors, that makes a lot more sense actually. I seem to recall the thought entering my brain but being dismissed for some reason. To be fixed.

75

Ah yes

I think there are basically 2 blockers before this can go in:

  1. Make the tests actually test something :)
  2. Use a member/getter for the result (see inline comment)

Otherwise LG for a first landing.

clang-rename/ClangRename.cpp
17–43

Not yet :) Patches welcome!

141–145

We really need to convert this to Diagnostics (fine in a follow-up patch though).

clang-rename/USRFinder.cpp
41

Use a class member and getter for the result.

clang-rename/USRLocFinder.cpp
43–47

This somehow sneaked back in?

test/clang-rename/VarTest.cpp
4–9

Ah, we need some adjustments:

  1. Add // REQUIRES: shell at the top
  2. copying the temp file only makes sense if we do in-place replacement; which I think we should do
  3. The CHECK comments get matched by the CHECKS. You'll want:

sed 's,//.*,,' %t.cpp | FileCheck %s

  1. Isn't that offset wrong? Both vim and

$ grep -FUbo 'foo;' /tmp/test
tell me the offset is 145. So, why not instead do
-offset=$(grep -FUbo 'foo;' %t.cpp |head -1 |cut -d: -f1)

unittests/clang-rename/USRLocFindingTest.cpp
77

I'm so so so sorry, but I lied about raw strings :'( - apparently VS 2012 doesn't support them yet, so they're not allowed in clang :( :( :(

mplant updated this revision to Diff 12463.Aug 13 2014, 11:19 AM

New diff!

clang-rename/ClangRename.cpp
17–43

Oh man I'll totally do that at some point.

141–145

That sounds fine for a follow up patch.

clang-rename/USRFinder.cpp
41

I already did! Funnily enough, you'll notice the Result argument is discarded, and the Result member is a single pointer! I just forgot to remove the constructor argument. oops!

clang-rename/USRLocFinder.cpp
43–47

Yeah oops.

Apart from the nits, I think this now LG :)

clang-rename/USRFinder.cpp
41

Heh :)

133–150

This now returns an uninitialized pointer if we find nothing... How about:

for (...) {
  if (...) {
    if (const NamedDecl *Result = Visitor.getNamedDecl())
      return Result;
    }
  }
}

return nullptr;

test/clang-rename/VarTest.cpp
2–5

I assume you checked that the test fails if you break the code? :)

mplant updated this revision to Diff 12464.Aug 13 2014, 11:39 AM

Fix for nits!

mplant edited the test plan for this revision. (Show Details)Aug 13 2014, 11:42 AM
mplant added inline comments.
clang-rename/USRFinder.cpp
133–150

Good catch!

test/clang-rename/VarTest.cpp
2–5

The realization that these had to go at the end because of the grep: >.<

klimek commandeered this revision.Aug 17 2014, 11:06 AM
klimek edited reviewers, added: mplant; removed: klimek.
klimek updated this revision to Diff 12590.Aug 17 2014, 11:07 AM

Various changes needed. This is FYI.

Hm, I wanted to upload the changes that I needed to make, but phab apparently doesn't handle that well... So you'll have to make sure to look at what I changed before submitting.

klimek accepted this revision.Aug 17 2014, 11:10 AM
klimek added a reviewer: klimek.
This revision is now accepted and ready to land.Aug 17 2014, 11:10 AM
klimek closed this revision.Aug 17 2014, 11:14 AM

Submitted as r215839. Note that
a) I had to do a couple of changes for this to compile without warning with latest head
b) I fixed the newFrontendActionFactory problem, please take a look how that works
c) the code is still grossly undertested; the next step will be to get test coverage up so people can work with the code; I just looked at a few places in USRFinder, and added FIXMEs; note that you'll need to do that for the rest of the code

Possible ways to add enough tests:

  1. get a good test coverage tool (although that will still miss a lot, but will be a good first indicator)
  2. reverse-TDD your code:

a) comment out all the code (*ALL* the code);
b) comment in enough code to make all tests pass (not that if you only need a single part of if (A || B), leave the part you don't need commented out (for example: if (A /* || B*/))
c) write test that forces you to uncomment part of the code
d) repeat until all your code is commented back in

If you have problems writing a test, please let me know.

Via Web