[cfe-dev] RFC: Easier AST Matching by Default

[Stephen Kelly via cfe-dev]

On 17/06/2020 22:47, Richard Smith wrote:
> Consider the example from Yitzhak Mandelbaum -- the issue there is 
> actually not whether we have a template or not; rather, it's a 
> difference between one- and two-argument constructors:
> I hope you can agree that this change in behavior, for an example such 
> as this, is a regression for the ability for beginners to understand 
> how matchers work and effectively write them!


Yes, I see your point. The current state doesn't account well for this 
case.

Would it resolve the problem if the CXXConstructExpr was not excluded in 
this case (in the initializer of a VarDecl), assuming that can be 
implemented?

I think it would remain better for newcomers not to see (and match on) 
the CXXConstructExprs in https://godbolt.org/z/TvUTgy .


>>     Perhaps as an alternative, we should not skip implicit nodes when
>>     trying to match a node that might be implicit (analogous to how
>>     Type::getAs steps over sugar nodes except when looking for that
>>     type sugar node), and instead of hiding implicit nodes in the AST
>>     dump we should dim them out but still show them?
>
>
>     Color wouldn't be particularly distinctive, especially in godbolt
>     for example, but it also would mean that the confusing stuff is
>     still *there* in the output. It can be overwhelming when
>     absolutely everything in the output is new and confusing.
>
>     (I also note that newcomers to Matchers don't need to see the AST
>     at all if the clang-query shows them the matchers they can use
>     instead: http://ce.steveire.com/z/9EF8x0 see my other posts on the
>     topic for more)
>
> Perhaps we should consider the AST dump to be an expert-level feature 
> and not try to optimize it for beginner usage, especially if we have 
> better output methods. I'm not sure, though; I think telling beginners 
> to look at the AST dump and match what they see there has been 
> somewhat effective.


Yes, I think it makes sense for newcomers to be able to get 
non-overwhelming exposure to the AST but not have to be confronted with 
it from the very beginning. That is where making clang-query output 
matchers could be useful as a beginning point. That is not yet upstream 
yet however.


> I think we should assume that we can ask the maintainers of compiler 
> explorer to render the output nicely, if we find that's a problem. 
> (They already apply some postprocessing to it, to remove pointer 
> values and the like.)

The "AST Output" on Compiler Explorer filters out pointers, but the 
clang-query tool does not. I have designs to have a way to make 
clang-query omit them anyway (using enable output simple-ast instead of 
detailed-ast - that is not upstream yet, but I think it means we don't 
need to do that kind of filtering in Compiler Explorer).

I think colors in the output might already show up properly, but that 
would still leave the problem of the things being *there*, even if they 
are greyed out or surrounded in square brackets (which I would find even 
more confusing and noisy).


>     Nevertheless, this change in the direction that I consider easier
>     hasn't been 100% popular among other clang developers (though I
>     wonder how much of that is due to being experts?). At this point,
>     if someone wishes to reverse the change of default I don't think I
>     would attempt to oppose it. It's not clear to me whether my vision
>     for making things easier for newcomers (which I set out in
>     EuroLLVM and which included ignoring these nodes) is shared. It
>     might be better at this point for others to decide.
>
> I think it's a good goal. But I think the implementation needs some 
> refinement. Did you have any thoughts on my suggestion of how to fix 
> this? (That is: try matching against implicit nodes, but skip them if 
> they don't match.) I don't think you commented on it.


I think the best refinement for now would be to restore the 
CXXConstructExpr in the case of a varDecl initializer, if that is 
possible (may not be).


If I understand your suggestion, given


```
struct B
{
   B(int);
};

B func1() {
   return 42;
}
```

a matcher like `returnStmt(hasReturnValue(fooExpr()))`

would match for `fooExpr` as any of


* exprWithCleanups()
* cxxConstructExpr() (twice)
* materializeTemporaryExpr()
* implicitCastExpr()
* integerLiteral()

right?

and a matcher like `returnStmt(hasReturnValue(expr().bind("theReturnVal")))`

would bind "theReturnVal" to the `exprWithCleanups()` right?

Your suggestion may be that it should bind to the integerLiteral() in 
that case. That may have more scope for confusion though?

Thanks,

Stephen


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200617/c971d25e/attachment.html>