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