[PATCH] Clang Rename Tool

Fri Aug 1 00:32:01 PDT 2014

On Fri, Aug 1, 2014 at 8:11 AM, Matthew Plant <mplant at google.com> wrote:

> Questions before (hopefully) substantial cleanup
>
> ================
> Comment at: clang-rename/ClangRename.cpp:66-75
> @@ +65,12 @@
> +                                       cl::cat(ClangRenameCategory));
> +static cl::opt<std::string>
> +    NewName("new-name", cl::desc("The new name to change the symbol to."),
> +            cl::cat(ClangRenameCategory));
> +static cl::opt<std::string> SymbolLocation(cl::Positional,
> +
> cl::desc("<file>:<line>:<column>"),
> +                                           cl::cat(ClangRenameCategory));
> +static cl::opt<unsigned> SymbolOffset(
> +    "offset",
> +    cl::desc("Locates the symbol by offset as opposed to
> <line>:<column>."),
> +    cl::cat(ClangRenameCategory));
> +static cl::list<std::string> FileNames(cl::Positional, cl::desc("[<file>
> ...]"),
> ----------------
> Manuel Klimek wrote:
> > 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.
> 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.
>

Note that this example already works with clang-check -fix.
Generally, when a tool has to interact with other tools, we found that the
offset based interface is the easiest to use for all sides - it's clear
that it's 0-based (and I've seen all kinds of combinations for line/column
based interfaces). When a tool somehow gets column based information, it's
also not too hard to do the conversion on its own.
Last, if it really turns out we need it, we can add it later - adding
interfaces is easy, removing them is hard.

> ================
> Comment at: clang-rename/ClangRename.cpp:131-133
> @@ +130,5 @@
> +  for (auto Loc : OldLocs) {
> +    // If the current location poihnts to the "operator" keyword, we want
> to
> +    // skip to the actual operator's spelling. We don't try to do this
> very; if
> +    // the operator isn't the next token over, we give up on the location.
> +    SmallVector<char, 32> Buffer;
> ----------------
> Manuel Klimek wrote:
> > Matthew Plant wrote:
> > > Manuel Klimek wrote:
> > > > Manuel Klimek wrote:
> > > > > points
> > > > Renaming operators seems ... strange; I would expect to add that (if
> at all) somewhen much later.
> > > 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.
> > "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.
> 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.
>

Ok, I can see that it's useful if you want to rename an operator into a
normal function call.

> 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.
>

I didn't expect you to vanish (although that has happened before ;), but
the maintenance burned is something that eats resources long-term (more
than you might imagine ...)

> ================
> Comment at: clang-rename/ClangRename.cpp:169
> @@ +168,3 @@
> +
> +    if (PrevName.empty()) {
> +      // Retrieve the previous name and USRs.
> ----------------
> Manuel Klimek wrote:
> > Matthew Plant wrote:
> > > Manuel Klimek wrote:
> > > > I find this rather strange architecturally - why don't we just visit
> the whole thing twice?
> > > 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.
> > 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.
> 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.
>

Ok, in that case I'd actually think we'll want to do one run over that file
with just the USR finder, and then do the rename refactoring run once we
have the USR.
The trick is that both parts should be completely independent, and only
rely on the USR as data being passed through, and I think that should be
reflected in the code structure (it'll also allow us to reuse each part
separately when needed).

> ================
> Comment at: clang-rename/ClangRename.cpp:248-258
> @@ +247,13 @@
> +static bool Rename() {
> +  CompilerInstance Compiler;
> +
> +  auto *Invocation = new CompilerInvocation;
> +  Compiler.setInvocation(Invocation);
> +
> +  // Make sure we're parsing the right language.
> +  Compiler.getLangOpts().CPlusPlus11 = true;
> +  Compiler.getLangOpts().GNUKeywords = true;
> +  Compiler.getLangOpts().NoBuiltin = false;
> +  Invocation->setLangDefaults(Compiler.getLangOpts(), IK_CXX,
> +                              LangStandard::lang_gnucxx11);
> +
> ----------------
> Manuel Klimek wrote:
> > Matthew Plant wrote:
> > > Manuel Klimek wrote:
> > > > Any reason you're not using RefactoringTool (which already does
> basically everything that's in this file)?
> > > 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
> > "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).
> 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?
>

The RefactoringTool is written to solve that exact problem...
You'll generate tooling::Replacements instead of directly interacting with
the Rewriter. The RefactoringTool will take care of everything under the
hood.

> 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.
>
> http://reviews.llvm.org/D4739
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140801/7351bd58/attachment.html>