[clang-rename] Do not print out error message upon encountering multiple replacements in the same SourceLocation.

[omtcyfz]

Summary

clang-rename might sometimes have multiple replacements at one SourceLocation, both trying to perform the same renaming. While this issue should be addressed properly at some point, for now it's fine to prevent clang-rename from printing out the error message to prevent confusion as it works just fine even if the error happens.

[arphaman]

clang-rename/RenamingAction.cpp

75

s/ingore/ignore/

76

Did you mean to use 'the' instead of 'there' here?

[omtcyfz]

Address two comments.

[omtcyfz]

@arphaman, thank you for the comments! Improved the wording.

[alexfh]

clang-rename/RenamingAction.cpp

73

Did you mean "adding a replacement fails..."?

75

"Specific header"? What exactly does it mean here?

[omtcyfz]

Address two comments from Alex.

[alexander-shaposhnikov]

clang-rename/RenamingAction.cpp

74

Let's consider the following example:
src/include/Point.h:

struct Point {};

src/a.cpp:

include <Point.h>

src/b.cpp:

include <Point.h>

clang-rename -qualified-name Point -new-name Point2 srcs/a.cpp srcs/b.cpp
Renaming failed in /Users/Alexshap/PlayRename/srcs/./include/Point.h! New replacement:
/Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2"
conflicts with existing replacement:
/Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2"

The thing is that clang-rename is trying to modify the same code twice (as in the example) and the return value (Error) of the method Replacements::add doesn't allow us to distinguish two cases: A. conflict (trying to apply different modifications to the same source code) B. (still conflict, but different) (trying to apply the same modification twice).
In the past when Replacements was a typedef on std::set and clang-rename was using insert(...) the case B wasn't an issue.
P.S. However (imo) the new (FIXME) comment seems to be a little bit misleading.

[omtcyfz]

clang-rename/RenamingAction.cpp

74

But the thing is that it's never the A. case. Unlike other refactorings, clang-rename doesn't introduce any name conflicts, it will only sometimes try to rename something multiple times.

[alexander-shaposhnikov]

clang-rename/RenamingAction.cpp

74

Yup

[omtcyfz]

Slightly change wording.

clang-rename/RenamingAction.cpp

74

Hence, I'm not sure what your concern is. Can you please elaborate?

[alexander-shaposhnikov]

clang-rename/RenamingAction.cpp

74

My concerns were about wording, the new version looks good to me.
I think the long-term fix would be to improve the interface of the class Replacements,
but it's clearly not in the scope of this patch (the other tools have the same issue),
so to me your diff is OK.

[omtcyfz]

clang-rename/RenamingAction.cpp

74

Eric actually has a patch for that: https://reviews.llvm.org/D24800

[omtcyfz]

Abandoning this, because rL282577, which introduces replacement deduplication, eliminates this issue. Big thanks to Eric!

clang-rename/RenamingAction.cpp

void HandleOneRename(ASTContext &Context, const std::string &NewName,
     unsigned PrevNameLen = PrevName.length();
     for (const auto &Loc : RenamingCandidates) {
       if (PrintLocations) {
         FullSourceLoc FullLoc(Loc, SourceMgr);
         errs() << "clang-rename: renamed at: " << SourceMgr.getFilename(Loc)
                << ":" << FullLoc.getSpellingLineNumber() << ":"
                << FullLoc.getSpellingColumnNumber() << "\n";
       }
-      // FIXME: better error handling.
       tooling::Replacement Replace(SourceMgr, Loc, PrevNameLen, NewName);
       llvm::Error Err = FileToReplaces[Replace.getFilePath()].add(Replace);
+      // FIXME: An error might happen here if USRLocFinder finds the symbol
+      // twice or if the symbol happens to be in a header included multiple
+      // times independently. Such error doesn't mean clang-rename failure and
+      // therefore should be suppressed. However, more careful error handling
+      // might be better.
       if (Err)
-        llvm::errs() << "Renaming failed in " << Replace.getFilePath() << "! "
-                     << llvm::toString(std::move(Err)) << "\n";
+        llvm::consumeError(std::move(Err));
     }
   }
 
 private:
   const std::vector<std::string> &NewNames, &PrevNames;
   const std::vector<std::vector<std::string>> &USRList;
   std::map<std::string, tooling::Replacements> &FileToReplaces;
   bool PrintLocations;