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