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

[Andy Soffer via Phabricator via cfe-commits]

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