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:
> > 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.
Sure. The lack of null check is consistent with the other two fields. so, that
just leaves it inconsistent with `Note`, which doesn't have as easy an answer
(since string isn't nullable). Given that `Note` isn't used (yet), let's punt
on the consistency issue until we deal with `Note` properly. You're good to go.
Thanks for talking this through!
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
