<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>