[PATCH] D91872: [libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

2020-11-20 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG88e62085624e: [libTooling] Update Transformers `node` 
combinator to include the trailing… (authored by ymandel).

Changed prior to commit:
  https://reviews.llvm.org/D91872?vs=306707=306729#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91872/new/

https://reviews.llvm.org/D91872

Files:
  clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
  clang/include/clang/Tooling/Transformer/RangeSelector.h
  clang/lib/Tooling/Transformer/RangeSelector.cpp
  clang/unittests/Tooling/TransformerTest.cpp


Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1353,7 +1353,7 @@
 
   // Changes the 'int' in 'S', but not the 'T' in 'TemplStruct':
   testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedField),
-changeTo(cat("safe_int ", name("theField",
+changeTo(cat("safe_int ", name("theField"), ";"))),
NonTemplatesInput + TemplatesInput,
NonTemplatesExpected + TemplatesInput);
 
@@ -1378,7 +1378,7 @@
 
   // Changes the 'int' in 'S', and (incorrectly) the 'T' in 'TemplStruct':
   testRule(makeRule(traverse(TK_AsIs, MatchedField),
-changeTo(cat("safe_int ", name("theField",
+changeTo(cat("safe_int ", name("theField"), ";"))),
 
NonTemplatesInput + TemplatesInput,
NonTemplatesExpected + IncorrectTemplatesExpected);
@@ -1589,7 +1589,7 @@
Changes[0].getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
-  EXPECT_EQ(format(*UpdatedCode), format(R"cc(;)cc"));
+  EXPECT_EQ(format(*UpdatedCode), "");
 
   ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
   EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
@@ -1598,8 +1598,7 @@
   Source, Changes[1].getReplacements());
   ASSERT_TRUE(static_cast(UpdatedCode))
   << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
-  EXPECT_EQ(format(*UpdatedCode), format(R"cc(#include "input.h"
-;)cc"));
+  EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
 }
 
 TEST_F(TransformerTest, AddIncludeMultipleFiles) {
Index: clang/lib/Tooling/Transformer/RangeSelector.cpp
===
--- clang/lib/Tooling/Transformer/RangeSelector.cpp
+++ clang/lib/Tooling/Transformer/RangeSelector.cpp
@@ -142,7 +142,8 @@
 Expected Node = getNode(Result.Nodes, ID);
 if (!Node)
   return Node.takeError();
-return Node->get() != nullptr && Node->get() == nullptr
+return (Node->get() != nullptr ||
+(Node->get() != nullptr && Node->get() == nullptr))
? tooling::getExtendedRange(*Node, tok::TokenKind::semi,
*Result.Context)
: CharSourceRange::getTokenRange(Node->getSourceRange());
Index: clang/include/clang/Tooling/Transformer/RangeSelector.h
===
--- clang/include/clang/Tooling/Transformer/RangeSelector.h
+++ clang/include/clang/Tooling/Transformer/RangeSelector.h
@@ -61,8 +61,8 @@
   return enclose(after(std::move(R1)), before(std::move(R2)));
 }
 
-/// Selects a node, including trailing semicolon (for non-expression
-/// statements). \p ID is the node's binding in the match result.
+/// Selects a node, including trailing semicolon, if any (for declarations and
+/// non-expression statements). \p ID is the node's binding in the match 
result.
 RangeSelector node(std::string ID);
 
 /// Selects a node, including trailing semicolon (always). Useful for selecting
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -148,7 +148,7 @@
   if (Options.get("Skip", "false") == "true")
 return None;
   return tooling::makeRule(clang::ast_matchers::functionDecl(),
-   change(cat("void nothing()")), cat("no message"));
+   changeTo(cat("void nothing();")), cat("no 
message"));
 }
 
 class ConfigurableCheck : public TransformerClangTidyCheck {


Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1353,7 +1353,7 @@

[PATCH] D91872: [libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

2020-11-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D91872#2408321 , @ymandel wrote:

> In D91872#2408278 , @aaron.ballman 
> wrote:
>
>> Drive-by question from the peanut gallery, sorry if this is an ignorant one 
>> -- not all declarations have a trailing semicolon; is that handled properly? 
>> e.g., `int x;` has a trailing semicolon but `int x, y;` only has a trailing 
>> semicolon for one of the two declarations. Relatedly, in `int f(int x);`, 
>> the declaration of `f` has a trailing semicolon, but the declaration of `x` 
>> does not.
>
> No, it's a good question -- the comments and the patch description should 
> have been clearer.  I've updated both. Also, I found a test that needed to be 
> fixed (and also, hopefully, illustrates why the new behavior is preferred. 
> The old behavior left the semicolon out of the replacement (which looks 
> weird) because it was working around the fact that the source being removed 
> didn't include the trailing semicolon, while the new version can just specify 
> the replacement correctly).
>
> Thanks!

Thanks, this is more clear now!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91872/new/

https://reviews.llvm.org/D91872

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91872: [libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

2020-11-20 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel added a comment.

In D91872#2408278 , @aaron.ballman 
wrote:

> Drive-by question from the peanut gallery, sorry if this is an ignorant one 
> -- not all declarations have a trailing semicolon; is that handled properly? 
> e.g., `int x;` has a trailing semicolon but `int x, y;` only has a trailing 
> semicolon for one of the two declarations. Relatedly, in `int f(int x);`, the 
> declaration of `f` has a trailing semicolon, but the declaration of `x` does 
> not.

No, it's a good question -- the comments and the patch description should have 
been clearer.  I've updated both. Also, I found a test that needed to be fixed 
(and also, hopefully, illustrates why the new behavior is preferred. The old 
behavior left the semicolon out of the replacement (which looks weird) because 
it was working around the fact that the source being removed didn't include the 
trailing semicolon, while the new version can just specify the replacement 
correctly).

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91872/new/

https://reviews.llvm.org/D91872

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91872: [libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

2020-11-20 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel updated this revision to Diff 306707.
ymandel added a comment.

Clarified that semicolons are only removed if present. Fixed test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91872/new/

https://reviews.llvm.org/D91872

Files:
  clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
  clang/include/clang/Tooling/Transformer/RangeSelector.h
  clang/lib/Tooling/Transformer/RangeSelector.cpp


Index: clang/lib/Tooling/Transformer/RangeSelector.cpp
===
--- clang/lib/Tooling/Transformer/RangeSelector.cpp
+++ clang/lib/Tooling/Transformer/RangeSelector.cpp
@@ -142,7 +142,8 @@
 Expected Node = getNode(Result.Nodes, ID);
 if (!Node)
   return Node.takeError();
-return Node->get() != nullptr && Node->get() == nullptr
+return (Node->get() != nullptr ||
+(Node->get() != nullptr && Node->get() == nullptr))
? tooling::getExtendedRange(*Node, tok::TokenKind::semi,
*Result.Context)
: CharSourceRange::getTokenRange(Node->getSourceRange());
Index: clang/include/clang/Tooling/Transformer/RangeSelector.h
===
--- clang/include/clang/Tooling/Transformer/RangeSelector.h
+++ clang/include/clang/Tooling/Transformer/RangeSelector.h
@@ -61,8 +61,8 @@
   return enclose(after(std::move(R1)), before(std::move(R2)));
 }
 
-/// Selects a node, including trailing semicolon (for non-expression
-/// statements). \p ID is the node's binding in the match result.
+/// Selects a node, including trailing semicolon, if any (for declarations and
+/// non-expression statements). \p ID is the node's binding in the match 
result.
 RangeSelector node(std::string ID);
 
 /// Selects a node, including trailing semicolon (always). Useful for selecting
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -148,7 +148,7 @@
   if (Options.get("Skip", "false") == "true")
 return None;
   return tooling::makeRule(clang::ast_matchers::functionDecl(),
-   change(cat("void nothing()")), cat("no message"));
+   changeTo(cat("void nothing();")), cat("no 
message"));
 }
 
 class ConfigurableCheck : public TransformerClangTidyCheck {


Index: clang/lib/Tooling/Transformer/RangeSelector.cpp
===
--- clang/lib/Tooling/Transformer/RangeSelector.cpp
+++ clang/lib/Tooling/Transformer/RangeSelector.cpp
@@ -142,7 +142,8 @@
 Expected Node = getNode(Result.Nodes, ID);
 if (!Node)
   return Node.takeError();
-return Node->get() != nullptr && Node->get() == nullptr
+return (Node->get() != nullptr ||
+(Node->get() != nullptr && Node->get() == nullptr))
? tooling::getExtendedRange(*Node, tok::TokenKind::semi,
*Result.Context)
: CharSourceRange::getTokenRange(Node->getSourceRange());
Index: clang/include/clang/Tooling/Transformer/RangeSelector.h
===
--- clang/include/clang/Tooling/Transformer/RangeSelector.h
+++ clang/include/clang/Tooling/Transformer/RangeSelector.h
@@ -61,8 +61,8 @@
   return enclose(after(std::move(R1)), before(std::move(R2)));
 }
 
-/// Selects a node, including trailing semicolon (for non-expression
-/// statements). \p ID is the node's binding in the match result.
+/// Selects a node, including trailing semicolon, if any (for declarations and
+/// non-expression statements). \p ID is the node's binding in the match result.
 RangeSelector node(std::string ID);
 
 /// Selects a node, including trailing semicolon (always). Useful for selecting
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -148,7 +148,7 @@
   if (Options.get("Skip", "false") == "true")
 return None;
   return tooling::makeRule(clang::ast_matchers::functionDecl(),
-   change(cat("void nothing()")), cat("no message"));
+   changeTo(cat("void nothing();")), cat("no message"));
 }
 
 class ConfigurableCheck : public TransformerClangTidyCheck {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91872: [libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

2020-11-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- 
not all declarations have a trailing semicolon; is that handled properly? e.g., 
`int x;` has a trailing semicolon but `int x, y;` only has a trailing semicolon 
for one of the two declarations. Relatedly, in `int f(int x);`, the declaration 
of `f` has a trailing semicolon, but the declaration of `x` does not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91872/new/

https://reviews.llvm.org/D91872

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91872: [libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

2020-11-20 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel created this revision.
ymandel added a reviewer: tdl-g.
Herald added a project: clang.
ymandel requested review of this revision.

Currently, `node` only includes the semicolon for (some) statements. However,
declarations have the same issue of trailing semicolons, so `node` should behave
the same for them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91872

Files:
  clang/include/clang/Tooling/Transformer/RangeSelector.h
  clang/lib/Tooling/Transformer/RangeSelector.cpp


Index: clang/lib/Tooling/Transformer/RangeSelector.cpp
===
--- clang/lib/Tooling/Transformer/RangeSelector.cpp
+++ clang/lib/Tooling/Transformer/RangeSelector.cpp
@@ -142,7 +142,8 @@
 Expected Node = getNode(Result.Nodes, ID);
 if (!Node)
   return Node.takeError();
-return Node->get() != nullptr && Node->get() == nullptr
+return (Node->get() != nullptr ||
+(Node->get() != nullptr && Node->get() == nullptr))
? tooling::getExtendedRange(*Node, tok::TokenKind::semi,
*Result.Context)
: CharSourceRange::getTokenRange(Node->getSourceRange());
Index: clang/include/clang/Tooling/Transformer/RangeSelector.h
===
--- clang/include/clang/Tooling/Transformer/RangeSelector.h
+++ clang/include/clang/Tooling/Transformer/RangeSelector.h
@@ -61,8 +61,8 @@
   return enclose(after(std::move(R1)), before(std::move(R2)));
 }
 
-/// Selects a node, including trailing semicolon (for non-expression
-/// statements). \p ID is the node's binding in the match result.
+/// Selects a node, including trailing semicolon (for declarations and
+/// non-expression statements). \p ID is the node's binding in the match 
result.
 RangeSelector node(std::string ID);
 
 /// Selects a node, including trailing semicolon (always). Useful for selecting


Index: clang/lib/Tooling/Transformer/RangeSelector.cpp
===
--- clang/lib/Tooling/Transformer/RangeSelector.cpp
+++ clang/lib/Tooling/Transformer/RangeSelector.cpp
@@ -142,7 +142,8 @@
 Expected Node = getNode(Result.Nodes, ID);
 if (!Node)
   return Node.takeError();
-return Node->get() != nullptr && Node->get() == nullptr
+return (Node->get() != nullptr ||
+(Node->get() != nullptr && Node->get() == nullptr))
? tooling::getExtendedRange(*Node, tok::TokenKind::semi,
*Result.Context)
: CharSourceRange::getTokenRange(Node->getSourceRange());
Index: clang/include/clang/Tooling/Transformer/RangeSelector.h
===
--- clang/include/clang/Tooling/Transformer/RangeSelector.h
+++ clang/include/clang/Tooling/Transformer/RangeSelector.h
@@ -61,8 +61,8 @@
   return enclose(after(std::move(R1)), before(std::move(R2)));
 }
 
-/// Selects a node, including trailing semicolon (for non-expression
-/// statements). \p ID is the node's binding in the match result.
+/// Selects a node, including trailing semicolon (for declarations and
+/// non-expression statements). \p ID is the node's binding in the match result.
 RangeSelector node(std::string ID);
 
 /// Selects a node, including trailing semicolon (always). Useful for selecting
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits