asoffer marked an inline comment as done.
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:
> 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();
> }
> ```
I don't see any particular reason to allow this field to take on it's null
value. A null check could essentially avoid attaching metadata, but that's also
what the default lambda would do here (because Any is also a nullable type).
That being said, sometimes I'm a bit UB-trigger-happy.
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
