[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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