<div dir="ltr"><div dir="ltr">On Wed, 17 Jun 2020 at 15:50, Stephen Kelly via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<p>On 17/06/2020 22:47, Richard Smith
wrote:<br></p>
<blockquote type="cite">
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:<br>
</blockquote>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>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!</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Yes, I see your point. The current state doesn't account well for
this case. <br>
</p>
<p>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? <br>
</p>
<p>I think it would remain better for newcomers not to see (and
match on) the CXXConstructExprs in <a href="https://godbolt.org/z/TvUTgy" target="_blank">https://godbolt.org/z/TvUTgy</a> .</p></div></blockquote><div>I think if someone writes a cxxConstructExpr matcher, they would expect it to match all cases that invoke a constructor. This case does, so it should match. I think it would be reasonable if, by default, both returnStmt(hasReturnValue(integerLiteral())) and returnStmt(hasReturnValue(cxxConstructExpr())) match this case. The former matches when viewed syntactically, and the latter matches when viewed semantically.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>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?</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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.</p>
<p>(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: <a href="http://ce.steveire.com/z/9EF8x0" target="_blank">http://ce.steveire.com/z/9EF8x0</a>
see my other posts on the topic for more)<br>
</p>
</div>
</blockquote>
<div>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.<br>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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.<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>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.)</div>
</div>
</div>
</blockquote>
<p>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).<br>
</p>
<p>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).<br>
</p>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<p> </p>
<p>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.</p>
</div>
</blockquote>
<div>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.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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).</p></div></blockquote><div>I think that is addressing a symptom rather than the cause. I think the root cause is that a matcher that is explicitly asking to match a certain implicit (or sometimes-implicit) AST node does not match. For example, I would expect that implicitCastExpr() *never* matches under the new default behavior. And I think that users, and especially beginner users, will see that as being simply broken -- if someone tries to write an implicitCastExpr() matcher, it's obvious that they want to match implicit casts, and we are not doing the user any favors by making that matcher not match by default.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<p>If I understand your suggestion, given</p>
<p><br>
```<br>
struct B<br>
{<br>
B(int);<br>
};<br>
<br>
B func1() {<br>
return 42;<br>
}<br>
```<br>
</p>
<p>a matcher like `returnStmt(hasReturnValue(fooExpr()))`<br>
</p>
<p>would match for `fooExpr` as any of<br>
</p>
<br>
* exprWithCleanups()<br>
* cxxConstructExpr() (twice)<br>
* materializeTemporaryExpr()<br>
* implicitCastExpr()<br>
* integerLiteral()
<p>right?<br></p></div></blockquote><div>Yes.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p>
</p>
<p>and a matcher like
`returnStmt(hasReturnValue(expr().bind("theReturnVal")))`</p>
<p>would bind "theReturnVal" to the `exprWithCleanups()` right?<br>
</p>
<p>Your suggestion may be that it should bind to the
integerLiteral() in that case. That may have more scope for
confusion though?</p></div></blockquote><div>I think that's an interesting question. My inclination would be to bind to the ExprWithCleanups. Simple syntax-oriented transformations should be fine with this; the source range of the implicit nodes should be the same as that of their inner nodes. And semantics-oriented transformations need this, in order to be able to inspect the semantics of the node they captured. (Fancy syntax-oriented transformations that are going to inspect the clang AST node they get back, rather than only applying matchers and rewrites, will need to be aware that they might get an implicit Expr* node. But once you're looking at the Expr* yourself, you've stepped out of the "beginner" area and are going to need to know more about how the Clang AST works in general.)<br></div></div></div>