================
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>
...]"),
----------------
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.
================
Comment at: clang-rename/ClangRename.cpp:90
@@ +89,3 @@
+// the input was given by a Clang getNameAsString method.
+static std::string getOpCallShortForm(const std::string &Call) {
+ const std::string Prefix = "operator";
----------------
Matthew Plant wrote:
> Manuel Klimek wrote:
> > I'd try to not use getNameAsString to do matching against - can't we solve
> > this in the type system?
> Not in a way that also conveniently gives us the operator spelling.
I thought there was a different way to get the operator name... I'll take a
look.
================
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;
----------------
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.
================
Comment at: clang-rename/ClangRename.cpp:169
@@ +168,3 @@
+
+ if (PrevName.empty()) {
+ // Retrieve the previous name and USRs.
----------------
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.
================
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);
+
----------------
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).
http://reviews.llvm.org/D4739
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
