[PATCH] D82226: Add Metadata to Transformer tooling

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Looks good! Only real question is one of design -- should we consider the (deeper) change of templating the various types rather than using dynamic typing?  For that matter, the decision doesn't even have to be the same for both AtomicChange and the Transformer types. Transformer could be templated while AtomicChange uses any.

I'm inclined towards what you have, but that may just be laziness on part (since this requires minimal changes).  I'm curious to hear what Dmitri thinks.



================
Comment at: clang/include/clang/Tooling/Refactoring/AtomicChange.h:144
   tooling::Replacements Replaces;
+  llvm::Any Metadata;
 };
----------------
Please add a field comment. Something like what you have in the revision description would be good.


================
Comment at: clang/lib/Tooling/Refactoring/AtomicChange.cpp:210
+    : AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
----------------
I assume that  
```
: AtomicChange(SM, KeyPosition), Metadata(std::move(M)) {}
```
didn't work?


================
Comment at: clang/unittests/Tooling/RefactoringTest.cpp:1299
 
+struct SomeMetadata {
+  int Data;
----------------
any reason not to use int directly as the type carried in the metadata?


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:460
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+      Factory->create(), Input, std::vector<std::string>(), "input.cc",
----------------
Why expect vs assert?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226