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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel 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 &) {
----------------
asoffer wrote:
> 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?
Sorry, I meant code that consumes this struct. Mostly that's just `translateEdits`. I realize now that `Note` is not a great example, since we totally ignore it. However, it is intended as optional, so if we had the code, it would be checking it for null first. Conversely, `TargetRange` and `Replacement` are assumed to never be null, yet we don't set them to default values here.  So, I think the first step is to decide if this is optional, and (if not) the second is to consistently handle non-optional values in this struct.

So, your code below would become something like:
```
llvm::Any Metadata;
if (E.Metadata != nullptr) {
  if (auto M = E.Metadata(Result))
    Metadata = std::move(*M);
  else
    return M.takeError();
}
```


================
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
----------------
asoffer wrote:
> 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.
Punting is fine with me. I'm not against making the user explicitly create the `Any`, but I don't have a strong opinion on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83820