clang-rename rename-all: support reading old/newname pairs from a YAML file

[vmiklos]

Summary

This is handy in case by the time clang-rename is invoked, an external
tool already genereated a list of oldname -> newname pairs to handle.

[vmiklos]

As a side note, this is the last feature that LibreOffice's simple clang-based rename tool (https://cgit.freedesktop.org/libreoffice/contrib/dev-tools/tree/clang/rename.cxx) supports, and clang-rename does not. (That one takes a CSV file, but in case -export-fixes outputs YAML, then here the input should be YAML as well for consistency.

[omtcyfz]

Still don't see general use case for that one, but since we already support multiple renamings "at once"...

Would also be nice to support at least offset + new-name in YAML input files, too.

clang-rename/tool/ClangRename.cpp

140

"Input" name doesn't make sense to me here.

docs/clang-rename.rst

70

Please move this block upwards [preferably right after line 42]. I consider information about limitations and editor more important than numerous clang-rename's terminal interface invocation examples.

test/clang-rename/ClassTestMultiByNameYAML.cpp

3

https://reviews.llvm.org/D23158 introduces simpler clang-rename invocations in tests. Please do that here, too.

[vmiklos]

clang-rename/tool/ClangRename.cpp

140

When you feed a YAML file into include-fixer, the argument name is -input, that's what I copied. What about -names?

[omtcyfz]

clang-rename/tool/ClangRename.cpp

140

Hm. That's true, include-fixer calls it the same. Well, then it'd be okay to let it be -input, so that it's more uniform across the tools.

[vmiklos]

Would also be nice to support at least offset + new-name in YAML input files, too.

Done.

Please move this block upwards [preferably right after line 42]. I consider
information about limitations and editor more important than numerous
clang-rename's terminal interface invocation examples.

Makes sense, done. I wouldn't mind if rename-all wouldn't be advertised (apart
from dumping the options) at all in the documentation, but then users would
have to look at the source code for what is the wanted YAML format, and *that*
would be bad, I guess. ;-)

https://reviews.llvm.org/D23158 introduces simpler clang-rename invocations
in tests. Please do that here, too.

Done.

[omtcyfz]

.cpp.rename-at.yaml? I understand it's done for the purpose of doing -input %s.rename-at.yaml, though I don't think getting proper names for them and either doing some string magic in tool invocation or just passing full name is wrong.

Other than that my concerns seem to be resolved. I'd wait for Manuel to jump in if he has time on Monday, though.

[omtcyfz]

+ I'll take a look again on Monday, too. May be too sleepy now :)

[omtcyfz]

Ah, and yes, it's better to move *.yaml to extra/test/clang-rename/Inputs in order to prevent confusion as every other file in extra/test/clang-rename is a test.

[vmiklos]

.cpp.rename-at.yaml?

I just discovered that lit provides %S that allows getting rid of the confusing
.cpp.yaml, I'm using that now.

Ah, and yes, it's better to move *.yaml to extra/test/clang-rename/Inputs

Done.

[omtcyfz]

clang-rename/tool/ClangRename.cpp

65

AFAIK there's no need to do that, integer types are by default initialized with 0, aren't they?

[vmiklos]

clang-rename/tool/ClangRename.cpp

65

Are you sure? Here is a minimal version that shows what goes wrong when that's not initialized explicitly: http://pastebin.com/raw/2ZsUgWf6 The "Use of uninitialised value of size 8" goes away with an explicit initialization.

[omtcyfz]

LGTM

clang-rename/tool/ClangRename.cpp

65

Well, in this case - yes. But in that case the default unsigned constructor isn't called.

What I meant is:

RenameAllInfo() : Offset(0) {}

• >

RenameAllInfo() : Offset() {}

In this case Offset is default-constructed. However, it is also true that one might argue that it's less "trivial" (whatever "triviality" definition would be :]). I guess it's fine to leave it like this.

[omtcyfz]

clang-rename/tool/ClangRename.cpp

65

Well, in this case - yes. But in that case the default unsigned constructor isn't called.

<to prevent confusion>

-> Well, in the case you sent - yes. But there is no default-construction there as there is no constructor with initializer list at all.

[vmiklos]

clang-rename/tool/ClangRename.cpp

65

Ah, I see what you mean. :-) I don't mind either way, but then I'll leave it as-is for now.

[alexfh]

A few late comments.

clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp

65 ↗

(On Diff #67381)

I'd use inline inititalizer instead.

73 ↗

(On Diff #67381)

Remove the stray slash.

152 ↗

(On Diff #67381)

Please use the explicit type, since it's not clear from the context.

156 ↗

(On Diff #67381)

Instead, I'd return the error code. The return value of this function seems to be propagated to the main's return value anyway.

clang-tools-extra/trunk/docs/clang-rename.rst

45 ↗

(On Diff #67381)

"Translation unit" is not a proper name, it shouldn't be capitalized.

[vmiklos]

In D23198#510269, @alexfh wrote:

A few late comments.

I've addressed these in r278201.

clang-rename/tool/ClangRename.cpp

 
 const char RenameAllUsage[] = "A tool to rename symbols in C/C++ code.\n\
 clang-rename renames every occurrence of a symbol named <old-name>.\n";
 
 static int renameAtMain(int argc, const char *argv[]);
 static int renameAllMain(int argc, const char *argv[]);
 static int helpMain(int argc, const char *argv[]);
 
+/// \brief An oldname -> newname rename.
+struct RenameAllInfo {
+  std::string OldName;
+  std::string NewName;
+};
+
+LLVM_YAML_IS_SEQUENCE_VECTOR(RenameAllInfo)
+
+namespace llvm {
+namespace yaml {
+
+/// \brief Specialized MappingTraits to describe how a RenameAllInfo is /
+/// (de)serialized.
+template <> struct MappingTraits<RenameAllInfo> {
+  static void mapping(IO &IO, RenameAllInfo &Info) {
+    IO.mapRequired("OldName", Info.OldName);
+    IO.mapRequired("NewName", Info.NewName);
+  }
+};
+
+} // end namespace yaml
+} // end namespace llvm
+
 int main(int argc, const char **argv) {
   if (argc > 1) {
     using MainFunction = std::function<int(int, const char *[])>;
     MainFunction Func = StringSwitch<MainFunction>(argv[1])
                             .Case("rename-at", renameAtMain)
                             .Case("rename-all", renameAllMain)
                             .Cases("-help", "--help", helpMain)
                             .Default(nullptr);
if (isRenameAll) {
     Usage = RenameAllUsage;
   } else {
     Category = &ClangRenameAtCategory;
     Usage = RenameAtUsage;
   }
 
   cl::list<std::string> NewNames(
       "new-name", cl::desc("The new name to change the symbol to."),
-      (isRenameAll ? cl::OneOrMore : cl::Required), cl::cat(*Category));
+      (isRenameAll ? cl::ZeroOrMore : cl::Required), cl::cat(*Category));
   cl::list<unsigned> SymbolOffsets(
       "offset",
       cl::desc("Locates the symbol by offset as opposed to <line>:<column>."),
       (isRenameAll ? cl::ZeroOrMore : cl::Required), cl::cat(*Category));
   cl::list<std::string> OldNames(
       "old-name",
       cl::desc(
           "The fully qualified name of the symbol, if -offset is not used."),
       (isRenameAll ? cl::ZeroOrMore : cl::Optional),
       cl::cat(ClangRenameAllCategory));
   cl::opt<bool> Inplace("i", cl::desc("Overwrite edited <file>s."),
                         cl::cat(*Category));
   cl::opt<bool> PrintName(
       "pn",
       cl::desc("Print the found symbol's name prior to renaming to stderr."),
       cl::cat(ClangRenameAtCategory));
   cl::opt<bool> PrintLocations(
       "pl", cl::desc("Print the locations affected by renaming to stderr."),
       cl::cat(ClangRenameAtCategory));
   cl::opt<std::string> ExportFixes(
       "export-fixes", cl::desc("YAML file to store suggested fixes in."),
       cl::value_desc("filename"), cl::cat(*Category));
+  cl::opt<std::string> Input(
+      "input", cl::desc("YAML file to load oldname-newname pairs from."),
+      cl::Optional, cl::cat(ClangRenameAllCategory));
 
   tooling::CommonOptionsParser OP(argc, argv, *Category, Usage);
 
+  if (!Input.empty()) {
+    // Populate OldNames and NewNames from a YAML file.
+    auto Buffer = llvm::MemoryBuffer::getFile(Input);
+    if (!Buffer) {
+      errs() << "clang-rename: failed to read " << Input << ": "
+             << Buffer.getError().message() << "\n";
+      exit(1);
+    }
+
+    std::vector<RenameAllInfo> Infos;
+    llvm::yaml::Input YAML(Buffer.get()->getBuffer());
+    YAML >> Infos;
+    for (const auto &Info : Infos) {
+      OldNames.push_back(Info.OldName);
+      NewNames.push_back(Info.NewName);
+    }
+  }
+
   // Check the arguments for correctness.
 
+  if (NewNames.empty()) {
+    errs() << "clang-rename: either -new-name or -input is required.\n\n";
+    exit(1);
+  }
+
   // Check if NewNames is a valid identifier in C++17.
   for (const auto &NewName : NewNames) {
     LangOptions Options;
     Options.CPlusPlus = true;
     Options.CPlusPlus1z = true;
     IdentifierTable Table(Options);
     auto NewNameTokKind = Table.get(NewName).getTokenID();
     if (!tok::isAnyIdentifier(NewNameTokKind)) {

docs/clang-rename.rst

 You can also identify one or more symbols to be renamed by giving the fully qualified
 name:
 
 .. code-block:: console
 
   $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
 
 
+Alternatively, old name / new name pairs can be put into a YAML file:
+
+.. code-block:: yaml
+
+  ---
+  - OldName:        foo
+    NewName:        bar
+  ...
+
+
+That way you can avoid spelling out all the names as commandline arguments:
+
+.. code-block:: console
+
+  $ clang-rename rename-all -input=test.yaml test.cpp
+
+
 The tool currently supports renaming actions inside a single Translation Unit
 only. It is planned to extend the tool's functionality to support multi-TU
 renaming actions in the future.
 
 :program:`clang-rename` also aims to be easily integrated into popular text
 editors, such as Vim and Emacs, and improve the workflow of users.
 
 Although a command line interface exists, it is highly recommended to use the
 text editor interface instead for better experience.
Generic Options:
     -version                   - Display the version of this program
 
   clang-rename rename-all options:
 
     -export-fixes=<filename>   - YAML file to store suggested fixes in.
     -extra-arg=<string>        - Additional argument to append to the compiler command line
     -extra-arg-before=<string> - Additional argument to prepend to the compiler command line
     -i                         - Overwrite edited <file>s.
+    -input=<string>            - YAML file to load oldname-newname pairs from.
     -new-name=<string>         - The new name to change the symbol to.
     -offset=<uint>             - Locates the symbol by offset as opposed to <line>:<column>.
     -old-name=<string>         - The fully qualified name of the symbol, if -offset is not used.
     -p=<string>                - Build path
 
 
 Vim Integration
 ===============

test/clang-rename/ClassTestMultiByNameYAML.cpp

+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -input %s.yaml %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};

test/clang-rename/ClassTestMultiByNameYAML.cpp.yaml

+---
+- OldName:        Foo1
+  NewName:        Bar1
+- OldName:        Foo2
+  NewName:        Bar2
+...