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