<div dir="ltr"><div>Pulling in more folks who had opinions in the original decision.</div><div><div>(y'all let me know if I misrepresented somebody's stance here)</div><div></div></div><div><br></div><div><div>Matt was the strongest one in favor of the new default mode, I believe, and he argued that this mode made several of his matchers more precise that were previously basically buggy, so I'd especially be interested whether your view changed based on this thread, Matt.</div><div></div></div><div><br></div><div>Sam I believe raised a similar idea as has been brought up in this thread, that if we want a syntactic matching mode, we should wait for SyntaxTrees to land before making matchers that work on SyntaxTrees.</div><div><br></div><div>Yitzhak was somewhere in between iirc.</div><div><br></div><div>Dmitri iiuc mainly would have liked a more thorough analysis of the possible actions before we switch.</div><div><br></div><div>I personally was convinced about the new default mode by Matt's apparent excitement, as well as the data from public clang-tidy matchers. I do think that I made a mistake by not pulling in the right set of folks earlier, though, for which I'm sorry :( </div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 22, 2020 at 8:52 AM Richard Smith via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<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 dir="ltr"><div dir="ltr">On Sun, 21 Jun 2020 at 13:51, Stephen Kelly via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">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 21/06/2020 19:59, Richard Smith
      wrote:<br></p>
    <blockquote type="cite">
      
      <div dir="ltr">
        <div>I think if we want to expose a syntactic mode, we should do
          that with a set of syntactic matchers (eg, a matcher that
          matches parenthesized initialization). Suppose someone wants
          to match all direct initialization. Right now, they need to do
          lots of checks: for a non-list, non-implicit cxxConstructExpr,
          for a varDecl whose initialization kind is direct init, for a
          cxxTemporaryObjectExpr, for a functionalCastExpr, and there'll
          likely be other kinds that I forgot and more added in the
          future.</div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>I agree there are missing matchers. I'd like to add them, but I
      don't think that's enough to make the matchers framework easier to
      use for newcomers.<br>
    </p>
    <p><br>
    </p>
    <blockquote type="cite">
      <div dir="ltr">
        <div class="gmail_quote">
          <div><br>
          </div>
          <div>I don't think it's intuitive to call
            IgnoreUnlessSpelledInSource mode Syntactic, because it isn't
            really that -- it doesn't let you match syntax, it lets you
            match semantics-with-associated-tokens (and even that is
            only an approximate description). </div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>Ok.<br>
    </p>
    <p><br>
    </p>
    <blockquote type="cite">
      <div dir="ltr">
        <div class="gmail_quote">
          <div>I think it would be really interesting to add a way to
            actually match syntax in a semantics-free way, but it seems
            like a big project.</div>
          <div><br>
          </div>
          <div>Also the mode you're calling Semantic is also not a
            semantic match, because it also matches syntax-only nodes.
            (It doesn't implicitly IgnoreParens or anything like that.)</div>
          <div><br>
          </div>
          <div>That said, I think neither Semantic nor Syntactic is the
            right default. By default we should be assuming that
            matchers want to match a combination of syntax and semantics
            -- usually a matcher will want to key off some semantic
            effect obtained in a particular syntactic context.</div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>I'm not certain I agree about that, or that it is what a newcomer
      wants, but ok.<br>
    </p>
    <p><br>
    </p>
    <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">
                    <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>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. </div>
                  </div>
                </div>
              </blockquote>
              <p><br>
              </p>
              <p>That is the purpose of AsIs/Semantic mode if I
                understood you correctly.</p>
              <p>The intention of the
                IgnoreUnlessSpelledInSource/Syntactic node is to *not*
                match certain implicit (or sometimes-implicit) AST
                nodes.<br>
              </p>
              <p><br>
              </p>
              <blockquote type="cite">
                <div dir="ltr">
                  <div class="gmail_quote">
                    <div>For example, I would expect that
                      implicitCastExpr() *never* matches under the new
                      default behavior. </div>
                  </div>
                </div>
              </blockquote>
              <p><br>
              </p>
              <p>That is correct.<br>
              </p>
              <p><br>
              </p>
              <blockquote type="cite">
                <div dir="ltr">
                  <div class="gmail_quote">
                    <div>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>
                  </div>
                </div>
              </blockquote>
              <p><br>
              </p>
              <p>Hmm, if someone is the kind of newcomer that they've
                never encountered a CallExpr, FunctionDecl or any other
                AST node in their life before, I don't see why
                implicitCastExpr() would be the first, or one of the
                first, things they try.</p>
            </div>
          </blockquote>
          <div>Perhaps because they want to match an implicit cast, and
            they find it in the documentation. Perhaps because they read
            one of the guides that says "look at the AST dump and write
            matchers to match what you see there".<br>
          </div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>I'm not sure which guide says that, but perhaps it should be
      updated to point people at clang-query. At least the guide I wrote
      does that:
<a href="https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-2-examining-the-clang-ast-with-clang-query/" target="_blank">https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-2-examining-the-clang-ast-with-clang-query/</a><br>
    </p>
    <p><br>
    </p>
    <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>There may be different/multiple levels of newcomer that
                we each have in mind here. <br>
              </p>
              <p>If someone is a kind of newcomer who wants to match on
                an implicitCastExpr(), then they're probably also the
                kind of newcomer who knows that's an implicit node and
                they should use Semantic node instead of Syntactic mode.
                Do you agree?</p>
            </div>
          </blockquote>
          <div>No. I think it's bad API design to have any mode in which
            implicitCastExpr compiles but doesn't ever match.</div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>Hmm, yes. Perhaps if the IgnoreUnlessSpelledInSource mode
      survives it should reject a matcher like that.</p>
    <p>----<br>
    </p>
    You've suggested a different behavior for matchers which I don't
    think anyone is working on (the design of or the implementation of).<br>
    <p>I continue to think the current behavior is sufficiently
      motivated by the examples in the RFC.<br>
    </p>
    <p>But, there's still tension about it.<br>
    </p>
    <p>So, where to from here? <br>
    </p>
    <p>Does the default have to be changed back to AsIs? Does
      IgnoreUnlessSpelledInSource have to be removed? Does the
      traverse() matcher have to be removed?</p></div></blockquote><div>I would like to hear the opinions of others on these questions. I think we've both described our perspectives and made our cases.</div><div><br></div><div><br></div><div>Just expanding on my position from earlier in this thread slightly: to directly address the "where to from here?" question, I would like us to get back to the state where, <i>in our default mode, the matcher for AST node X is always able to match all instances of AST node X</i>. I think there are various options for how we get there that seem acceptable, and that don't sacrifice your noble goals of making AST matching easier and more intuitive. In particular, I think it's fine (and probably good) if by default we look through non-matching implicit nodes while looking for a matching node (so long as we don't look past a matching node just because it's implicit). And I think it's fine (and probably good) to expose an easy way to explicitly control whether we automatically look through implicit nodes or not. But I think if no-one is prepared to do the work to make our default mode satisfy the above property while still looking through implicit nodes if (and only if) they don't match, then I think we should change the default back to the state we had before. (I don't have much of an opinion on whether to keep or remove 'traverse' and its various modes, other than that we've already caused a substantial amount of churn with the changes thus far, and removing them again would cause further churn.)</div></div></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div></div>