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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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