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

[Andy Soffer via Phabricator via cfe-commits]

asoffer added inline comments.


================
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 &) {
----------------
ymandel wrote:
> nit: typos
> 
> The same applies to `Note`. Since this is a nullable type, can we ask that the user check `Metadata != nullptr`?
I don't think I'm understanding the question. Where/when would users check for Metadata != nullptr?

Currently in this diff, any library construction of the Metadata field will be non-null. Users would have to explicitly pass null in if they wanted it to be null which would pretty quickly break when the generator is called, so this seems like a not-too-big foot-gun? Does that answer the question?


================
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
----------------
ymandel wrote:
> 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`.
I think you're asking about why the generic callable instead of a std::function or LLVM equivalent type. It's not about implicit conversions but rather about deduction. Type deduction allows only for conversions on qualifiers (T to const T&, for example). So if we specified a std::function, it would either require:
1. The cannot pass lambdas without explicitly wrapping them in a std::function construction so that the return type can be deduced.
2. We have an explicit std::function<llvm::Any(...)>, but then the user-provided callable would have to return an llvm::Any explicitly.
Neither of these seem ideal.

The Expected-unwrapping requires some ugly SFINAE, so I'd prefer to punt on this until we feel the need to have Expected's in metadata.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83820