ymandel added a comment.
Looks like your changes to the .cpp and test files were reverted...
================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:93
+ // Not all transformations will want or need to attach metadata and therefore
+ // sholud not be requierd to do so.
AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
----------------
nit: typos
The same applies to `Note`. Since this is a nullable type, can we ask that the
user check `Metadata != nullptr`?
================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:270
-// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+// FIXME: If `Metadata` returns an `llvm::Expected<T>` the `AnyGenerator` will
+// construct an `llvm::Expected<llvm::Any>` where no error is present but the
----------------
I think I understand now. Is the idea that we'll only get one implicit
construction from the compiler, so we want to use that "free" conversion on the
`llvm::Any`, rather than on the `llvm::Expected`, which would force the user to
explicitly construct the `llvm::Any`?
I think we should address this with separate functions (or overloads at least),
one for the case of never-fail metadata constructors and one for failable
constructors. That said, any computation that takes the matchresult is prone to
failure because of unbound nodes. so, I would expect it to be common for the
Callable to be failable. however, if you feel that's the uncommon case, let's
optimize towards the common case, like you've done.
With all that said, I agree that if the Callable returns an `Expected` we
should rewrap correctly, not bury in `Any`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83820/new/
https://reviews.llvm.org/D83820
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
