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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
