Page MenuHomePhabricator

Record whether a AST Matcher constructs a Node
AcceptedPublic

Authored by steveire on Nov 11 2018, 2:36 PM.

Details

Reviewers
aaron.ballman
Summary

These matchers are bindable. Recording this information will make it
possible to introspect the matchers which can be used inside another
matcher.

Diff Detail

Event Timeline

steveire created this revision.Nov 11 2018, 2:36 PM
aaron.ballman added inline comments.Nov 11 2018, 3:47 PM
lib/ASTMatchers/Dynamic/Registry.cpp
70

Fix matchDescriptor for coding conventions, but please don't reuse the name of a type when fixing it. :-)

76

Same comment here.

77

Mildly not keen on the use of auto here. It's a factory function, so it kind of names the resulting type, but it also looks like the type will be ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type from the template argument, which is incorrect.

79

Elide braces.

88

Same matchDescriptor here.

89

Similar comment about auto here.

90

Elide braces.

steveire added inline comments.Nov 12 2018, 12:06 PM
lib/ASTMatchers/Dynamic/Registry.cpp
77

There is no reason to assume that taking a template argument means that type is result.

The method is getFrom which decreases the ambiguity still further.

Spelling out the type doesn't add anything useful. This should be ok.

Aside from the uses of auto and the lack of tests, this LGTM.

lib/ASTMatchers/Dynamic/Registry.cpp
77

There is no reason to assume that taking a template argument means that type is result.

Aside from all the other places that do exactly that (getAs<>, cast<>, dyn_cast<>, castAs<>, and so on). Generally, when we have a function named get that takes a template type argument, the result when seen in proximity to auto is the template type.

Spelling out the type doesn't add anything useful. This should be ok.

I slept on this one and fall on the other side of it; using auto hides information that tripped up at least one person when reading the code, so don't use auto. It's not clear enough what the resulting type will be.

steveire added inline comments.Nov 12 2018, 3:42 PM
lib/ASTMatchers/Dynamic/Registry.cpp
77

I put this in the category of requiring

SomeType ST = SomeType::create();

instead of

auto ST = SomeType::create();

ast_type_traits::ASTNodeKind is already on that line and you want to see it again.

zturner added inline comments.
lib/ASTMatchers/Dynamic/Registry.cpp
77

FWIW I'm with Aaron here. Im' not familiar at all with this codebase, and looking at the code, my first instinct is "the result type is probably ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type". According to Aaron's earlier comment, that is incorrect, so there's at least 1 data point that it hinders readability.

So, honest question. What *is* the return type here?

aaron.ballman added inline comments.Nov 12 2018, 3:54 PM
lib/ASTMatchers/Dynamic/Registry.cpp
77

So, honest question. What *is* the return type here?

Much to my surprise, it's ASTNodeKind. It was entirely unobvious to me that this was a factory function.

steveire added inline comments.Nov 12 2018, 3:55 PM
lib/ASTMatchers/Dynamic/Registry.cpp
77

@zturner Quoting myself:

ast_type_traits::ASTNodeKind is already on that line and you want to see it again.

The expression is getFromNodeKind. There is a pattern of SomeType::fromFooKind<FooKind>() returning a SomeType. This is not so unusual.

zturner added inline comments.Nov 12 2018, 4:03 PM
lib/ASTMatchers/Dynamic/Registry.cpp
77

Note that at the top of this file there's already a using namespace clang::ast_type_traits; So if you're worried about verbosity, then you can remove the ast_type_traits::, remove the auto, and the net effect is that the overall line will end up being shorter.

steveire added inline comments.Nov 13 2018, 1:56 AM
lib/ASTMatchers/Dynamic/Registry.cpp
77

The funny thing is - if you see a line like

Parser CodeParser = Parser::getFromArgs(Args);

you have no idea what type Parser is!

To start, it could be clang::Parser or clang::ast_matchers::dynamic::Parser, depending on a using namespace which might appear any distance up in the file. It is not uncommon for clang files to be thousands of lines lone.

Or it could be inside a template with template<typename Parser>, or there could be a using Parser = Foo; any distance up in the file.

Parser CodeParser = Parser::getFromArgs(Args);

is no more helpful than

auto CodeParser = Parser::getFromArgs(Args);

Sticking with the same example, if CodeParser is a field, then you might have a line

CodeParser = Parser::getFromArgs(Args);

and you could object and create a macro which expands to nothing to ensure that the type appears on the line

CodeParser = CALL_RETURNS(Parser)Parser::getFromArgs(Args);

No one does that, because it is ok for the type to not appear on the line.

You would also have to object to code such as

Object.setParser(Parser::getFromArgs(Args));

requiring it to instead be

Parser CodeParser = Parser::getFromArgs(Args);
Object.setParser(CodeParser);

so that you can read the type.

Even then, what if those two lines are separated by a full page of code? How do you know the type of CodeParser in the Object.setParser(CodeParser) call? The answer is you don't and you don't need to.

You would also require

return Parser::getFromArgs(Args);

to instead be

Parser CodeParser = Parser::getFromArgs(Args);
return CodeParser;

Claims that human readers need to see a type are as incorrect as they are inconsistent.

I don’t really have much more to add here except to refer you to the style
guide:

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Specifically this line: “Use auto if and only if it makes the code more
readable or easier to maintain.”

Given that 2 out of 2 reviewers who have looked at this have said it did
not make the code more readable or easier to maintain for them , and have
further said that they feel the return type is not “obvious from the
context”, i do not believe this usage fits within our style guidelines.

I don’t think it’s worth beating on this anymore though, because this is a
lot of back and forth over something that doesn’t actually merit this much
discussion. So in the interest of conforming to the style guide above,
please remove auto and then start a thread on llvm-dev if you disagree with
the policy

steveire marked an inline comment as done.Nov 27 2018, 2:06 AM
steveire added inline comments.
lib/ASTMatchers/Dynamic/Registry.cpp
85

You still can't see the return type of getFromNodeKind() here, but I trust that this is fine nonetheless.

aaron.ballman accepted this revision.Nov 27 2018, 5:38 AM

LGTM! Please commit with whatever patch makes use of nodeConstructors() (as this functionality doesn't stand on its own).

This revision is now accepted and ready to land.Nov 27 2018, 5:38 AM