This is an archive of the discontinued LLVM Phabricator instance.

Change metadata to deferred evalutaion in Clang Transformer.
ClosedPublic

Authored by asoffer on Jul 14 2020, 2:57 PM.

Details

Summary

Metadata is being changed from an llvm::Any to a MatchConsumer<llvm;:Any> so that it's evaluation can be be dependent on on MatchResults passed in.

Diff Detail

Event Timeline

asoffer created this revision.Jul 14 2020, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 2:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr2 accepted this revision.Jul 15 2020, 4:58 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang/include/clang/Tooling/Transformer/RewriteRule.h
270–290

What is "this" who is "we"? Should this be an implementation comment instead (in the function body next to the relevant line)?

This revision is now accepted and ready to land.Jul 15 2020, 4:58 AM
ymandel accepted this revision.Jul 15 2020, 8:34 AM
ymandel added inline comments.
clang/include/clang/Tooling/Transformer/RewriteRule.h
92–96

Why default, especially when we don't default the others? Is it worth a comment?

271

Why generalize to support any callable? Perhaps worth explaining in comment?

asoffer updated this revision to Diff 278262.Jul 15 2020, 11:44 AM

Add comments describing why we provide defaults for Metadata generation and design of withMetadata function template.

asoffer marked 4 inline comments as done.Jul 15 2020, 11:50 AM
asoffer added inline comments.
clang/include/clang/Tooling/Transformer/RewriteRule.h
92–96

Added comments.

270–290

Added comments.

asoffer marked 3 inline comments as done.Jul 15 2020, 11:51 AM

Looks like your changes to the .cpp and test files were reverted...

clang/include/clang/Tooling/Transformer/RewriteRule.h
93

nit: typos

The same applies to Note. Since this is a nullable type, can we ask that the user check Metadata != nullptr?

269–270

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.

asoffer updated this revision to Diff 278315.Jul 15 2020, 2:41 PM
asoffer marked 2 inline comments as done.

Typo fix.

clang/include/clang/Tooling/Transformer/RewriteRule.h
93

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?

269–270

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.

ymandel marked an inline comment as done.Jul 16 2020, 5:33 AM
ymandel added inline comments.
clang/include/clang/Tooling/Transformer/RewriteRule.h
93

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();
}
269–270

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.

asoffer updated this revision to Diff 278552.Jul 16 2020, 11:25 AM

Fix formatting.

asoffer marked an inline comment as done.Jul 16 2020, 11:29 AM
asoffer added inline comments.
clang/include/clang/Tooling/Transformer/RewriteRule.h
93

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.

ymandel added inline comments.Jul 16 2020, 12:26 PM
clang/include/clang/Tooling/Transformer/RewriteRule.h
93

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!

93

typo: requierd

asoffer updated this revision to Diff 278611.Jul 16 2020, 3:04 PM
asoffer marked an inline comment as done.

Typo fix.

This revision was automatically updated to reflect the committed changes.
ymandel reopened this revision.Jul 21 2020, 5:48 AM
This revision is now accepted and ready to land.Jul 21 2020, 5:48 AM
asoffer updated this revision to Diff 279572.Jul 21 2020, 10:32 AM

Construct types for default metadata explicitly.

This revision was automatically updated to reflect the committed changes.