<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 27, 2020 at 9:21 PM Stephen Kelly 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 bgcolor="#FFFFFF">
<p><br>
</p>
<div>On 27/05/2020 15:24, Sam McCall wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>Hi Stephen,</div>
<div><br>
</div>
<div>First off: I think we've mitigated this enough on our end
that we're not in immediate need of changes beyond fixing the
problems found in Transformers.</div>
<div>Thanks for being so engaged on this, it helped a lot
clarifying our options and I think we can help make this
smoother for adopters when this gets released.</div>
</div>
</blockquote>
<p><br>
</p>
<p>Great!<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div>I've also had a chance to sync internally with Dmitri,
Haojian, Yitzhak and Manuel and get their perspectives too.</div>
<div><br>
</div>
<div>Some brief notes on our local fixes:</div>
<div>Haojian wrote a (matcher/transformer-based!) tool to
identify the "roots" where matchers were passed into consuming
clang APIs and mechanically wrapped them with traverse() at
that point.</div>
</div>
</blockquote>
<p><br>
</p>
<p>Sounds good. There's something MC-Escher-like pleasing about
that.<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div>This was applied across the codebase, whether needed to
make tests pass or not. We have no plans to systematically
remove AsIs from these - tool owners can do this (or not) as
part of maintenance.</div>
<div>We chose this approach because:</div>
<div> - there was preference for expressing this subtle
difference next to the matcher logic rather than "burying" it
in tool framework stuff. Otherwise there's little prospect of
ever using the new mode, and copy/pasting matchers would give
mysteriously different results.</div>
</div>
</blockquote>
<p><br>
</p>
<p>Yes, I listed the "burying" method as something small and quick
that would be quick to unblock others, but not a long-term
solution.<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div> - it was amenable to automated refactoring and thus
scaling (more-so than modifying tool entry points) because
there were a few identifiable points to instrument.</div>
<div> - applying it to everything meant not worrying about
whether tests were adequate</div>
<div> - updating all tools to use the new traversal mode
probably isn't a good use of time due to the long tail.</div>
<div><br>
</div>
<div>There were ~40 direct uses of ASTContext::getParents(),
whose behaviour was also changed, and these were significantly
more work to adapt. (Apparently. I wasn't the one doing the
work :-))</div>
<div>I suppose the trivial/safe fix is to set and restore the
mode before each call, but this seems pretty gross - this
doesn't seem like something that should require mutating
global state.</div>
</div>
</blockquote>
<p><br>
</p>
<p>Yes, I agree it has a bit of a smell. There is a <span style="font-size:11pt;font-family:"Courier New";color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap" id="gmail-m_-7614231120349700852docs-internal-guid-e63db8ba-7fff-436d-90eb-4f050da6c219">TraversalKindScope</span>
RAII type for use in a few checks as it was only needed in
`::check` implementations.<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>How do you feel about making the old behaviour and the
traversal-mode-respecting version of this function both
reasonably available, with matchers calling the new one?</div>
<div>I don't have a strong opinion on naming/structure, but
would prefer existing getParents() calls to either get the
existing behaviour or a build break forcing a choice.</div>
</div>
</blockquote>
<p><br>
</p>
<p>That sounds fine to me.<br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>We had some discussion of the direction of the change
overall (I know it's late for this). Making matchers easier is
great. Some concern that you really have to know the AST to
use matchers effectively</div>
</div>
</blockquote>
<p>[Side note:<br>
</p>
<p>This is something of a side-note to this thread, but I don't
think you do need to be familiar with AST structure in order to
use AST Matchers. At least not in the future. <br>
</p>
<p>Consider</p>
<p> <a href="https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590" target="_blank">https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590</a><br>
</p>
<p>versus</p>
<p> <a href="https://steveire.files.wordpress.com/2018/11/allenabled.png?w=900&h=590" target="_blank">https://steveire.files.wordpress.com/2018/11/allenabled.png?w=900&h=590</a><br>
</p>
<p>The latter is what we have today. The former does not require
users to be familiar with the AST because clang-query can tell you
what matchers and source locations you can use with binding
results:<br>
</p>
<p> <a href="http://ce.steveire.com/z/Kbk8Yj" target="_blank">http://ce.steveire.com/z/Kbk8Yj</a></p>
<p>I demo'd these clang-query features in November 2018 but I
haven't been able to get the patches upstream and I haven't seen
interest or drive from the community in getting the patches
actually approved unfortunately. <br></p></div></blockquote><div><br></div><div>I'm sorry about your experience.</div><div> </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> <a href="https://steveire.wordpress.com/2019/01/02/refactor-with-clang-tooling-at-codedive-2018/" target="_blank">https://steveire.wordpress.com/2019/01/02/refactor-with-clang-tooling-at-codedive-2018/</a><br>
</p>
<p>If you have a declarative debugger for your matchers you don't
need to debug via an trip through the AST either:<br>
</p>
<p> <a href="http://ce.steveire.com/z/Miznm_" target="_blank">http://ce.steveire.com/z/Miznm_</a><br>
</p>
<p>I demo'd that one at EuroLLVM 2019 along with a Proof of Concept
Qt UI for this stuff:
<a href="https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/" target="_blank">https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/</a><br>
</p>
<p>It is definitely not necessary for novices to see an AST dump or
to be very familiar with it before starting. It might be the
current state in LLVM upstream, but it hasn't needed to be true
for almost 1.5 years, depending on how you see it :).</p>
<p>
</p>
<p>There is a huge amount more that we could do to improve
mechanical refactoring tools for novices and others. But I haven't
been able to drive that forward because I can't get my changes
reviewed upstream in a timeframe that reflects a drive from
upstream to get them in. That means I can't stay engaged and I
have to disappear for months. Better is possible :).<br></p></div></blockquote><div>For what it's worth, I'd love to see your contributions go in. The problem is that initially that will mean lots of round trips and rewriting, which takes time. This usually gets better over time, as you get more fluid in how code in clang is reviewed / written, and also when you start being an owner for the areas you've contributed enough in to be the expert on them.</div><div> </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>If you or someone you know have interest in any of this and have
drive to actually get them upstream (this is the critical part -
the desire to approve the patches) please let me know.</p>
<p>End Side Note]<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div>, and this might give the illusion you only need to know
syntax. But some pushback against this idea:
ignoreImplicit.... is a hassle even for experienced people.</div>
</div>
</blockquote>
<p>Yes.</p>
<p>Thanks,</p>
<p>Stephen<br>
</p>
</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>