<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<div class="moz-cite-prefix">On 17/01/2020 12:54, Dmitri Gribenko
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">On Fri, Jan 17, 2020 at 12:41 PM Stephen Kelly
<<a href="mailto:steveire@gmail.com" moz-do-not-send="true">steveire@gmail.com</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><br>
</p>
<div>On 13/01/2020 16:39, Dmitri Gribenko wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">On Fri, Jan 10, 2020 at 4:34 PM Aaron
Ballman via cfe-dev <<a
href="mailto:cfe-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">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">I've not seen
much to suggest the community is not in favor of
this<br>
direction, so I think you're all set to post
patches for review. If<br>
there are community concerns, we can address them
during the review<br>
process. Thank you for working on this!<br>
</blockquote>
<div><br>
</div>
<div>Sorry, I just came back from vacation. I'm not
sure if it is a good idea, in particular, for the
following reasons:</div>
<div><br>
</div>
<div>- The document specifies the goals in terms of
a solution: "Goal: Users should not have to
account for implicit AST nodes when writing AST
Matchers". <br>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>The goal is to make AST Matchers more accessible/easy
to use for newcomers/non-experts in the clang AST by
no-longer confronting them with 100% of the complexity
on first use.<br>
</p>
<p>I think it's important to bridge that gap. See my
EuroLLVM talk for more.<br>
</p>
</div>
</blockquote>
<div>I totally agree with the goal of making AST Matchers,
refactoring, and tooling more accessible (which is why I'm
helping with libraries like Stencil and Syntax trees). </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>This change affects AST Matchers, but as far as I can see, it
does not affect any other effort. <br>
</p>
<p>I don't see why you mention those initiatives in this and your
other email. We're only talking about AST Matchers. Unless AST
Matchers are to be removed, we should improve them, regardless of
Syntax Tree, Stencil and Transformer.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>However the goal that I quoted from the proposal on this
thread ("Goal: Users should not have to account for implicit
AST nodes when writing AST Matchers") constrains the
solution to exactly what is being proposed.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Perhaps there's a misunderstanding. That's the goal of changing
the default.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<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>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>- The complexity for implementers (although it
also applies to <a
href="https://reviews.llvm.org/D61837"
target="_blank" moz-do-not-send="true">https://reviews.llvm.org/D61837</a>,
which landed a while ago, and I didn't see it).
Allowing to run the same matcher in multiple modes
increases our support and testing matrix.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>I don't think it increases the test matrix more than
any other feature. Can you expand on this?<br>
</p>
</div>
</blockquote>
<div><a href="https://reviews.llvm.org/D61837"
moz-do-not-send="true">https://reviews.llvm.org/D61837</a>
makes it possible to run every matcher with different
traversal semantics. Our current tests only test one mode
for each matcher, the default mode. There are two ways to
define custom matchers: the advanced way (through the
AST_MATCHER macro) that is needed when you want to traverse
the AST yourself, and a simpler way -- by combining existing
matchers.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>AFAICT, defining a matcher using the macro, or defining it as a
composition function doesn't change much?<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Consider the `makeIteratorLoopMatcher` defined in
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp.
The caller can run the newly made matcher in any traversal
mode, even though the matcher was probably written with the
default mode in mind.</div>
<div><br>
</div>
<div>Clang codebase does not have many matchers that are
combinations of existing matchers</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>It does actually - callee() for example. The fact that is uses a
macro doesn't change anything AFAICT. Also alignOfExpr() which
does not use a macro, but I don't see how that changes anything. <br>
</p>
<p>Am I missing something?<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>, but code that uses Clang as a library (and often
written by engineers who are not compiler experts and just
need to get some refactoring done) does that very often in
my experience working at Google.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>I don't think the traverse() matcher changes so much in this
area. "Just need to get some refactoring done" sounds like writing
a composition matcher within a check, not writing it for some
library code.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<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>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>- The complexity cliff for users. Many
non-trivial matchers need to explicitly manage
implicit AST nodes.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>You just described the complexity cliff which is (after
my change) not the first thing a newcomer hits.</p>
<p>It's good to move that cliff back so that the newcomer
is not confronted with it immediately.<br>
</p>
</div>
</blockquote>
<div>As soon as you pretty-print the AST (which I think most
people should do before writing a matcher), you see implicit
nodes.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>If using clang-query, you can change the mode of how clang-query
matches and dumps ASTs. But, in the future you won't need to dump
the AST anyway, because clang-query will just tell you the
available matchers and what they match. <br>
</p>
<p>See my EuroLLVM talk for more, in particular the workflow diagram
which does not include inspecting the AST:
<a class="moz-txt-link-freetext" href="https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590">https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590</a><br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<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>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>- The cost of ClangTidy. ClangTidy runs all
matchers simultaneously in one AST traversal. If
we implement implicit/no-implicit modes as
separate AST traversals, ClangTidy would need to
run two traversals, one for "easier" matchers
(skipping implicit code), and one for "expert"
matchers (traversing implicit code). We would also
need to build two parent maps.</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
<p>As of <a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D73388">https://reviews.llvm.org/D73388</a> we no longer create a
parent map for each traversal kind used at runtime.<br>
</p>
<p>This means I can no longer measure a runtime cost of introducing
use of the traverse() matcher in clang-tidy.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<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>That said, I think there is some scope for optimization
in my approach. The optimizations I have in mind (being
more efficient with parent maps) wouldn't apply to
Syntax Trees though as that is a completely separate
system.<br>
</p>
<p><br>
</p>
</div>
</blockquote>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Done, per above.<br>
</p>
<p> </p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<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>As long as the current AST Matchers APIs exist, they
should be improved. Perhaps they will be less important
in the future when we have Syntax Trees for these
use-cases instead. However, for now, I don't think the
ease of use improvement in my patches should be
prevented based existence of a future alternative
system.<br>
</p>
</div>
</blockquote>
<div>I agree that we should not block improvements on
existence of a future system. However, each specific change
should be evaluated based on its merits and costs. I think
there are non-trivial downsides to offering a different mode
in existing AST matchers, which create maintenance and
testing costs that we will be paying perpetually. </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>We may not achieve your agreement on this change, but I think we
have consensus already that this change should proceed.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>It instantly creates a huge testing gap for existing AST
matchers (which we currently test only in one mode). </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>The traverse() matcher does not introduce a need to test each AST
matcher in each mode.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>It also creates traps for existing code that will be able
to start accidentally calling existing matchers in a
traversal mode that they were not designed for.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Yes, true. It is possible to write a matcher which works as
someone expects in one mode but not the other, such as a matcher
which specifically expects to match a implicitCastExpr().</p>
<p>This can matter for matchers which are in "libraries", but not
for matchers which are implementation details inside checks.
Unless a particular check exposes the traversal kind as an option,
those matchers can not be affected. Any check which does expose
that as an option has to do so consciously and explicitly and has
to test it. Nothing new there.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>On the other hand, my objection primarily applies to <a
href="https://reviews.llvm.org/D61837"
moz-do-not-send="true">https://reviews.llvm.org/D61837</a>,
which already landed, not to changing the default. </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Understood. Hopefully that comes through in my email.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CA+Y5xYd0QpWZZjG+zg1tfo09dDFSxu4az52LhYoqVz_ENjC7TQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>If you want to still proceed with changing the default,
my request would be to provide detailed guidance (probably
as a section in release notes) about updating existing code
that uses matchers, including ClangTidy checkers.</div>
</div>
</div>
</blockquote>
<p>Certainly.</p>
<p><br>
</p>
<p>Thanks,</p>
<p><br>
</p>
<p>Stephen.</p>
<p><br>
</p>
</body>
</html>