[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2033fa29b09f: Add documentation illustrating use of
IgnoreUnlessSpelledInSource (authored by stephenkelly).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91639/new/
https://reviews.llvm.org/D91639
Files:
clang/docs/LibASTMatchersReference.html
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -17,6 +17,12 @@
color: blue;
cursor: pointer;
}
+span.mono { font-family: monospace; }
+
+.traverse_compare, .traverse_compare td, .traverse_compare th {
+ border: 1px solid black;
+ border-collapse: collapse;
+}
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for the new documentation! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91639/new/ https://reviews.llvm.org/D91639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
steveire updated this revision to Diff 306634.
steveire added a comment.
Update
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91639/new/
https://reviews.llvm.org/D91639
Files:
clang/docs/LibASTMatchersReference.html
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -17,6 +17,12 @@
color: blue;
cursor: pointer;
}
+span.mono { font-family: monospace; }
+
+.traverse_compare, .traverse_compare td, .traverse_compare th {
+ border: 1px solid black;
+ border-collapse: collapse;
+}
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
steveire added inline comments. Comment at: clang/docs/LibASTMatchersReference.html:91-94 +This mode is hard to use correctly and +requires more development iteration because it means +that the user must write AST Matchers to explicitly traverse or ignore nodes +which are not visible. aaron.ballman wrote: > I was trying to reword this to avoid making a judgmental statement that the > default mode is hard to use correctly -- I'd strike this part of the > paragraph. WDYT? Oops, that's what I was trying to do, but accidentally left the previous stuff behind. Gone now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91639/new/ https://reviews.llvm.org/D91639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
aaron.ballman added inline comments. Comment at: clang/docs/LibASTMatchersReference.html:91-94 +This mode is hard to use correctly and +requires more development iteration because it means +that the user must write AST Matchers to explicitly traverse or ignore nodes +which are not visible. I was trying to reword this to avoid making a judgmental statement that the default mode is hard to use correctly -- I'd strike this part of the paragraph. WDYT? Comment at: clang/docs/LibASTMatchersReference.html:303 + + Replacement of `begin()` with `cbegin()`: + steveire wrote: > aaron.ballman wrote: > > Backticks won't help here -- should probably use tags. > `pre` is equivalent to three backticks. I added a `span` for use within > paragraphs. Thank you for translating my suggestion into the one I was trying to make. :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91639/new/ https://reviews.llvm.org/D91639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
steveire added inline comments. Comment at: clang/docs/LibASTMatchersReference.html:303 + + Replacement of `begin()` with `cbegin()`: + aaron.ballman wrote: > Backticks won't help here -- should probably use tags. `pre` is equivalent to three backticks. I added a `span` for use within paragraphs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91639/new/ https://reviews.llvm.org/D91639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
steveire updated this revision to Diff 306494.
steveire added a comment.
Update
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91639/new/
https://reviews.llvm.org/D91639
Files:
clang/docs/LibASTMatchersReference.html
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -17,6 +17,12 @@
color: blue;
cursor: pointer;
}
+span.mono { font-family: monospace; }
+
+.traverse_compare, .traverse_compare td, .traverse_compare th {
+ border: 1px solid black;
+ border-collapse: collapse;
+}
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
aaron.ballman added a comment.
Thank you for the documentation effort!
Comment at: clang/docs/LibASTMatchersReference.html:100
+
+Traverse Mode
+
I think this section should go above the Node Matchers section in the document
so that Node Matchers stays directly above the table of node matchers.
Comment at: clang/docs/LibASTMatchersReference.html:104
+The default mode of operation of AST Matchers visits all nodes in the AST,
+even if they are not spelled in the source. This is AsIs mode. This mode is
+hard to use correctly and requires more development iteration because it means
Can you add `` tags around `AsIs` and `IgnoreUnlessSpelledInSource` to
make it clear that these are sort of syntactic constructs rather than prose?
Comment at: clang/docs/LibASTMatchersReference.html:105-107
+hard to use correctly and requires more development iteration because it means
+that the user must write AST Matchers to explicitly traverse or ignore nodes
+which are not visible.
Rather than say the default mode is hard to use correctly, how about:
`This mode requires writing AST matchers that explicitly traverse or ignore
implicit nodes, such as parentheses surrounding an expression or expressions
with cleanups. These implicit nodes are not always obvious from the syntax of
the source code, and so this mode requires careful consideration and testing to
get the desired behavior from an AST matcher.`
Comment at: clang/docs/LibASTMatchersReference.html:116
+the source using the IgnoreUnlessSpelledInSource mode. This is likely to be far
+less error-prone for users who are not already very familiar with the AST. It
is
+also likely to be less error-prone for experienced AST users, as difficult
cases
familiar with the AST -> familiar with where implicit nodes appear in the AST
Comment at: clang/docs/LibASTMatchersReference.html:128
+
+When using the C++ API such as in clang-tidy checks, the traverse() matcher
+is used to set the mode:
tags around `traverse()`
Comment at: clang/docs/LibASTMatchersReference.html:153
+
+struct B
+{
Should we consistently switch this style to:
```
struct B {
```
to save on some vertical whitespace (it doesn't impact all of the rows, but
will help in some cases)?
Comment at: clang/docs/LibASTMatchersReference.html:288
+
+Match found! Insertion produces incorrect output:
+
How about switching the exclamation points into full stops (here and elsewhere)?
Comment at: clang/docs/LibASTMatchersReference.html:303
+
+ Replacement of `begin()` with `cbegin()`:
+
Backticks won't help here -- should probably use tags.
Comment at: clang/docs/LibASTMatchersReference.html:312-318
+void foo()
+{
+ const Container c;
+ c.begin();
+
+ for (auto i : c)
+ {
Some more potential places for vertical whitespace savings.
Actually, would it be too onerous to ask you to run all of the code snippets
through clang-format with the LLVM style so that they're consistently formatted
and trims some of the vertical whitespace?
Comment at: clang/docs/LibASTMatchersReference.html:356
+
+ Replacement of int member with safe_int:
+
Could use some tags around the code constructs.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91639/new/
https://reviews.llvm.org/D91639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
steveire updated this revision to Diff 305863.
steveire added a comment.
Update
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91639/new/
https://reviews.llvm.org/D91639
Files:
clang/docs/LibASTMatchersReference.html
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -96,6 +96,471 @@
+
+Traverse Mode
+
+
+The default mode of operation of AST Matchers visits all nodes in the AST,
+even if they are not spelled in the source. This is AsIs mode. This mode is
+hard to use correctly and requires more development iteration because it means
+that the user must write AST Matchers to explicitly traverse or ignore nodes
+which are not visible.
+
+In addition, because template instantations are matched in the default mode,
+transformations can be accidentally made to template declarations. Finally,
+because implicit nodes are matched by default, transformations can be made on
+entirely incorrect places in the code.
+
+For these reasons, it is possible to ignore AST nodes which are not spelled in
+the source using the IgnoreUnlessSpelledInSource mode. This is likely to be far
+less error-prone for users who are not already very familiar with the AST. It is
+also likely to be less error-prone for experienced AST users, as difficult cases
+do not need to be encountered and matcher expressions adjusted for these
+cases.
+
+In clang-query, the mode can be changed with
+
+set traversal IgnoreUnlessSpelledInSource
+
+
+This affects both matchers and AST dump output in results.
+
+When using the C++ API such as in clang-tidy checks, the traverse() matcher
+is used to set the mode:
+
+Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
+ returnStmt(hasReturnArgument(integerLiteral(equals(0
+ ), this);
+
+
+The following table compares the AsIs mode with the IgnoreUnlessSpelledInSource
+mode:
+
+
+
+.traverse_compare, .traverse_compare td, .traverse_compare th {
+ border: 1px solid black;
+ border-collapse: collapse;
+}
+
+
+
+AsIsIgnoreUnlessSpelledInSource
+
+
+ AST dump of func1:
+
+struct B
+{
+ B(int);
+};
+
+B func1() {
+ return 42;
+}
+
+
+
+
+C++98 dialect:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-ExprWithCleanups
+ `-CXXConstructExpr
+`-MaterializeTemporaryExpr
+ `-ImplicitCastExpr
+`-ImplicitCastExpr
+ `-CXXConstructExpr
+`-IntegerLiteral 'int' 42
+
+C++11, C++14 dialect:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-ExprWithCleanups
+ `-CXXConstructExpr
+`-MaterializeTemporaryExpr
+ `-ImplicitCastExpr
+`-CXXConstructExpr
+ `-IntegerLiteral 'int' 42
+
+C++17, C++20 dialect:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-ImplicitCastExpr
+ `-CXXConstructExpr
+`-IntegerLiteral 'int' 42
+
+
+
+All dialects:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-IntegerLiteral 'int' 42
+
+
+
+
+Matcher for returned '42':
+
+struct B
+{
+ B(int);
+};
+
+B func1() {
+ return 42;
+}
+
+
+
+
+All dialects:
+
+returnStmt(hasReturnValue(
+ignoringImplicit(
+ignoringElidableConstructorCall(
+ignoringImplicit(
+cxxConstructExpr(hasArgument(0,
+ignoringImplicit(
+integerLiteral().bind("returnVal")
+)
+))
+)
+)
+)
+))
+
+
+All dialects:
+
+returnStmt(hasReturnValue(
+integerLiteral().bind("returnVal")
+))
+
+
+
+Match result for
+implicitCastExpr()
+given:
+
+struct B
+{
+ B(int);
+};
+
+B func1() {
+ return 42;
+}
+
+
+
+
+Match found.
+
+No match.
+
+
+ Match result for:
+
+cxxConstructorDecl(
+ isCopyConstructor()
+ ).bind("prepend_explicit")
+
+given:
+
+struct Other {
+};
+struct Copyable {
+ Other m_o;
+ Copyable();
+};
+
+
+
+Match found! Insertion produces incorrect output:
+
+struct Other {
+};
+struct explicit Copyable {
+ Other m_o;
+ Copyable();
+};
+
+
+
+No match found. Incorrect replacement not possible.
+
+
+
+ Replacement of `begin()` with `cbegin()`:
+
+cxxMemberCallExpr(
+ on(ConstContainerExpr),
+ callee(cxxMethodDecl(hasName("begin")))
+ ).bind("replace_with_cbegin")
+
+given:
+
+void foo()
+{
+ const Container c;
+ c.begin();
+
+ for (auto i : c)
+ {
+
+ }
+}
+
+
+
+2 matches found! Replacement produces incorrect output:
+
+void foo()
+{
+ const Container c;
+ c.cbegin();
+
+ for (auto i :.cbegin() c)
+ {
+
+ }
+}
+
+
+
+1 match found! Replacement produces correct output:
+
+void foo()
+{
+ const Container c;
+ c.cbegin();
+
+ for (auto i : c)
+ {
+
+ }
+}
+
+
+
+
+ Replacement of int member with safe_int:
+
+fieldDecl(
+ hasType(asString("int"))
+ ).bind("use_safe_int")
+
+given:
+
+struct S {
+ int m_i;
+};
[PATCH] D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource
steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
steveire requested review of this revision.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91639
Files:
clang/docs/LibASTMatchersReference.html
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -96,6 +96,337 @@
+
+Traverse Mode
+
+
+The default mode of operation of AST Matchers visits all nodes in the AST,
+even if they are not spelled in the source. This is AsIs mode. This mode is
+hard to use correctly and requires more development iteration because it means
+that the user must write AST Matchers to explicitly traverse or ignore nodes
+which are not visible.
+
+In addition, because template instantations are matched in the default mode,
+transformations can be accidentally made to template declarations. Finally,
+because implicit nodes are matched by default, transformations can be made on
+entirely incorrect places in the code.
+
+For these reasons, it is possible to ignore AST nodes which are not spelled in
+the source using the IgnoreUnlessSpelledInSource mode. This is likely to be far
+less error-prone for users who are not already very familiar with the AST. It is
+also likely to be less error-prone for experienced AST users, as difficult cases
+do not need to be encountered and matcher expressions adjusted for these
+cases.
+
+In clang-query, the mode can be changed with
+
+set traversal IgnoreUnlessSpelledInSource
+
+
+This affects both matchers and AST dump output in results.
+
+When using the C++ API such as in clang-tidy checks, the traverse() matcher
+is used to set the mode:
+
+Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
+ returnStmt(hasReturnArgument(integerLiteral(equals(0
+ ), this);
+
+
+The following table compares the AsIs mode with the IgnoreUnlessSpelledInSource
+mode:
+
+
+
+.traverse_compare, .traverse_compare td, .traverse_compare th {
+ border: 1px solid black;
+ border-collapse: collapse;
+}
+
+
+
+AsIsIgnoreUnlessSpelledInSource
+
+
+ AST dump of func1:
+
+struct B
+{
+ B(int);
+};
+
+B func1() {
+ return 42;
+}
+
+
+
+
+C++98 dialect:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-ExprWithCleanups
+ `-CXXConstructExpr
+`-MaterializeTemporaryExpr
+ `-ImplicitCastExpr
+`-ImplicitCastExpr
+ `-CXXConstructExpr
+`-IntegerLiteral 'int' 42
+
+C++11, C++14 dialect:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-ExprWithCleanups
+ `-CXXConstructExpr
+`-MaterializeTemporaryExpr
+ `-ImplicitCastExpr
+`-CXXConstructExpr
+ `-IntegerLiteral 'int' 42
+
+C++17, C++20 dialect:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-ImplicitCastExpr
+ `-CXXConstructExpr
+`-IntegerLiteral 'int' 42
+
+
+
+All dialects:
+
+FunctionDecl
+`-CompoundStmt
+ `-ReturnStmt
+`-IntegerLiteral 'int' 42
+
+
+
+
+Matcher for returned '42':
+
+struct B
+{
+ B(int);
+};
+
+B func1() {
+ return 42;
+}
+
+
+
+
+All dialects:
+
+returnStmt(hasReturnValue(
+ignoringImplicit(
+ignoringElidableConstructorCall(
+ignoringImplicit(
+cxxConstructExpr(hasArgument(0,
+ignoringImplicit(
+integerLiteral().bind("returnVal")
+)
+))
+)
+)
+)
+))
+
+
+All dialects:
+
+returnStmt(hasReturnValue(
+integerLiteral().bind("returnVal")
+))
+
+
+
+Match result for
+implicitCastExpr()
+given:
+
+struct B
+{
+ B(int);
+};
+
+B func1() {
+ return 42;
+}
+
+
+
+
+Match found.
+
+No match.
+
+
+ Match result for:
+
+cxxConstructorDecl(
+ isCopyConstructor()
+ ).bind("prepend_explicit")
+
+given:
+
+struct Other {
+};
+struct Copyable {
+ Other m_o;
+ Copyable();
+};
+
+
+
+Match found! Insertion produces incorrect output:
+
+struct Other {
+};
+struct explicit Copyable {
+ Other m_o;
+ Copyable();
+};
+
+
+
+No match found. Incorrect replacement not possible.
+
+
+
+ Replacement of `begin()` with `cbegin()`:
+
+cxxMemberCallExpr(
+ on(ConstContainerExpr),
+ callee(cxxMethodDecl(hasName("begin")))
+ ).bind("replace_with_cbegin")
+
+given:
+
+void foo()
+{
+ const Container c;
+ c.begin();
+
+ for (auto i : c)
+ {
+
+ }
+}
+
+
+
+2 matches found! Replacement produces incorrect output:
+
+void foo()
+{
+ const Container c;
+ c.cbegin();
+
+ for (auto i :.cbegin() c)
+ {
+
+ }
+}
+
+
+
+1 match found! Replacement produces correct output:
+
+void foo()
+{
+ const Container c;
+ c.cbegin();
+
+ for (auto i : c)
+ {
+
+ }
+}
+
+
+
+
+ Replacement of int member with safe_int:
+
+fieldDecl(
+ hasType(asString("int"))
+
