[PATCH] D82226: Add Metadata to Transformer tooling

[Andy Soffer via Phabricator via cfe-commits]

asoffer marked 2 inline comments as done.
asoffer added a comment.

In D82226#2114111 <https://reviews.llvm.org/D82226#2114111>, @ymandel wrote:

> 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 think the tradeoff here is 
Dynamic typing -- faster compile times, type safety checked at run-time (in tests), lower maintenance cost
Templates -- Faster runtime, type safety checked at compile-time, better user expereience

I don't think the runtime speed is particularly important here in the sense that it's unlikely to be the bottleneck. Given the crash-on-invalid-type semantics, we're essentially trading off usability for maintenance cost. In this case I don't think asking users to do the extra cast is a significant enough burden to warrant the cost of templating everything.

I also don't strongly object to it. The nice thing about llvm::Any, is that this can be mostly changed incrementally if we later decide that's appropriate (only having to change the one getMetadata call at the call-site).



================
Comment at: clang/lib/Tooling/Refactoring/AtomicChange.cpp:210
+    : AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
----------------
ymandel wrote:
> I assume that  
> ```
> : AtomicChange(SM, KeyPosition), Metadata(std::move(M)) {}
> ```
> didn't work?
Correct. You can't use a forwarding constructor followed by a field initializer (which I learned by attempt it on this PR).


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:460
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+      Factory->create(), Input, std::vector<std::string>(), "input.cc",
----------------
ymandel wrote:
> Why expect vs assert?
EXPECT when the test can reasonably continue to produce useful output without crashing. So here, if the tool fails, we can still safely access Changes, but if Changes.size() != 1, we can't safely index into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82226