[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
ymandel marked an inline comment as done.
Closed by commit rL366473: [LibTooling] Relax Transformer to allow rewriting 
macro expansions (authored by ymandel, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64518?vs=210576=210630#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64518

Files:
  cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
  cfe/trunk/unittests/Tooling/TransformerTest.cpp

Index: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
@@ -36,37 +36,6 @@
 
 using MatchResult = MatchFinder::MatchResult;
 
-// Did the text at this location originate in a macro definition (aka. body)?
-// For example,
-//
-//   #define NESTED(x) x
-//   #define MACRO(y) { int y  = NESTED(3); }
-//   if (true) MACRO(foo)
-//
-// The if statement expands to
-//
-//   if (true) { int foo = 3; }
-//   ^ ^
-//   Loc1  Loc2
-//
-// For SourceManager SM, SM.isMacroArgExpansion(Loc1) and
-// SM.isMacroArgExpansion(Loc2) are both true, but isOriginMacroBody(sm, Loc1)
-// is false, because "foo" originated in the source file (as an argument to a
-// macro), whereas isOriginMacroBody(SM, Loc2) is true, because "3" originated
-// in the definition of MACRO.
-static bool isOriginMacroBody(const clang::SourceManager ,
-  clang::SourceLocation Loc) {
-  while (Loc.isMacroID()) {
-if (SM.isMacroBodyExpansion(Loc))
-  return true;
-// Otherwise, it must be in an argument, so we continue searching up the
-// invocation stack. getImmediateMacroCallerLoc() gives the location of the
-// argument text, inside the call text.
-Loc = SM.getImmediateMacroCallerLoc(Loc);
-  }
-  return false;
-}
-
 Expected>
 tooling::detail::translateEdits(const MatchResult ,
 llvm::ArrayRef Edits) {
@@ -75,14 +44,17 @@
 Expected Range = Edit.TargetRange(Result);
 if (!Range)
   return Range.takeError();
-if (Range->isInvalid() ||
-isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
+llvm::Optional EditRange =
+getRangeForEdit(*Range, *Result.Context);
+// FIXME: let user specify whether to treat this case as an error or ignore
+// it as is currently done.
+if (!EditRange)
   return SmallVector();
 auto Replacement = Edit.Replacement(Result);
 if (!Replacement)
   return Replacement.takeError();
 tooling::detail::Transformation T;
-T.Range = *Range;
+T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
 Transformations.push_back(std::move(T));
   }
Index: cfe/trunk/unittests/Tooling/TransformerTest.cpp
===
--- cfe/trunk/unittests/Tooling/TransformerTest.cpp
+++ cfe/trunk/unittests/Tooling/TransformerTest.cpp
@@ -137,7 +137,7 @@
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@
   testRule(ruleStrlenSize(), Input, Input);
 }
 
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
-  std::string Input = R"cc(
-#define ID(e) e
-int f(string s) { return ID(strlen(s.c_str())); })cc";
-  std::string Expected = R"cc(
-#define ID(e) e
-int f(string s) { return ID(REPLACED); })cc";
-  testRule(ruleStrlenSize(), Input, Expected);
-}
-
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
@@ -619,23 +608,114 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
+  std::string Input = R"cc(
+#define ZERO 0
+int f(string s) { return ZERO; }
+  )cc";
+  std::string Expected = R"cc(
+#define ZERO 0
+int f(string s) { return 999; }
+  )cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+   change(node(zero), text("999")));
+  testRule(R, Input, Expected);
+}
+
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
   std::string Input = R"cc(
 #define MACRO(str) 

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 210576.
ymandel added a comment.

remove unneeded include


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518

Files:
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -137,7 +137,7 @@
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@
   testRule(ruleStrlenSize(), Input, Input);
 }
 
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
-  std::string Input = R"cc(
-#define ID(e) e
-int f(string s) { return ID(strlen(s.c_str())); })cc";
-  std::string Expected = R"cc(
-#define ID(e) e
-int f(string s) { return ID(REPLACED); })cc";
-  testRule(ruleStrlenSize(), Input, Expected);
-}
-
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
@@ -619,23 +608,114 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
+  std::string Input = R"cc(
+#define ZERO 0
+int f(string s) { return ZERO; }
+  )cc";
+  std::string Expected = R"cc(
+#define ZERO 0
+int f(string s) { return 999; }
+  )cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+   change(node(zero), text("999")));
+  testRule(R, Input, Expected);
+}
+
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
-int f(string s) { return MACRO(s); })cc";
-  testRule(ruleStrlenSize(), Input, Input);
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define MACRO(str) strlen((str).c_str())
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten.
+TEST_F(TransformerTest, MacroArg) {
+  std::string Input = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(REPLACED); }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
 }
 
-// This test handles the corner case where a macro called within another macro
-// expands to matching code, but the matched code is an argument to the nested
-// macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
-// will get this wrong, and transform the code. This test verifies that no such
-// transformation occurs.
-TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+// Tests that expressions in macro arguments can be rewritten, even when the
+// macro call occurs inside another macro's definition.
+TEST_F(TransformerTest, MacroArgInMacroDef) {
   std::string Input = R"cc(
 #define NESTED(e) e
 #define MACRO(str) NESTED(strlen((str).c_str()))
-int f(string s) { return MACRO(s); })cc";
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests the corner case of the identity macro, specifically that it is
+// discarded in the rewrite rather than preserved (like PLUS is preserved in the
+// previous test).  This behavior is of dubious value (and marked with a FIXME
+// in the code), but we test it to verify (and demonstrate) how this case is
+// handled.
+TEST_F(TransformerTest, IdentityMacro) {
+  std::string Input = R"cc(
+#define ID(e) e
+int f(string s) { return ID(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define ID(e) e
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// No rewrite is applied when the changed text does not encompass the entirety
+// of the expanded text. That is, the edit would have to be applied to the
+// macro's definition to succeed and editing the expansion point would not
+// suffice.
+TEST_F(TransformerTest, NoPartialRewriteOMacroExpansion) {
+  

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 3 inline comments as done.
ymandel added inline comments.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional
+makeValidRange(CharSourceRange Range, const MatchResult ) {
+  const SourceManager  = *Result.SourceManager;

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Could we add unit tests for this particular function?
> > > 
> > > Interesting cases (`[[` and `]]` mark the start and end of a range):
> > > ```
> > > #define FOO(a) a+a;
> > > #define BAR 10+
> > > 
> > > // change part of a macro argument
> > > int a = FOO([[10]] + 10);
> > > 
> > > // change the whole macro expansion
> > > int b = [[FOO(10+10)]];
> > > 
> > > // Try to change 10 inside 'BAR', but not '+'.
> > > // Should this fail? Should we give a warning?
> > > int c = BAR 3; 
> > > 
> > > // Try changing the lhs (10) of a binary expr, but not rhs.
> > > // Is that allowed? Should we give a warning?
> > > int d = FOO(10);
> > > ```
> > Sure. What do you think of exposing this function in 
> > clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from 
> > there?
> Sounds reasonable. Was thinking of a better name, maybe something like 
> `getRangeForEdit()`?
> Would also suggest to accept `SourceManager` and `LangOptions` instead of 
> `MatchResult` to narrow down the requirements on the clients.
Went with passing the ASTContext rather than the MatchResult (in the new 
revision D64924)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 210575.
ymandel added a comment.

Moved `makeValidRange` to its own revision and rebased onto that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518

Files:
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -137,7 +137,7 @@
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@
   testRule(ruleStrlenSize(), Input, Input);
 }
 
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
-  std::string Input = R"cc(
-#define ID(e) e
-int f(string s) { return ID(strlen(s.c_str())); })cc";
-  std::string Expected = R"cc(
-#define ID(e) e
-int f(string s) { return ID(REPLACED); })cc";
-  testRule(ruleStrlenSize(), Input, Expected);
-}
-
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
@@ -619,23 +608,114 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
+  std::string Input = R"cc(
+#define ZERO 0
+int f(string s) { return ZERO; }
+  )cc";
+  std::string Expected = R"cc(
+#define ZERO 0
+int f(string s) { return 999; }
+  )cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+   change(node(zero), text("999")));
+  testRule(R, Input, Expected);
+}
+
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
-int f(string s) { return MACRO(s); })cc";
-  testRule(ruleStrlenSize(), Input, Input);
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define MACRO(str) strlen((str).c_str())
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten.
+TEST_F(TransformerTest, MacroArg) {
+  std::string Input = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(REPLACED); }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
 }
 
-// This test handles the corner case where a macro called within another macro
-// expands to matching code, but the matched code is an argument to the nested
-// macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
-// will get this wrong, and transform the code. This test verifies that no such
-// transformation occurs.
-TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+// Tests that expressions in macro arguments can be rewritten, even when the
+// macro call occurs inside another macro's definition.
+TEST_F(TransformerTest, MacroArgInMacroDef) {
   std::string Input = R"cc(
 #define NESTED(e) e
 #define MACRO(str) NESTED(strlen((str).c_str()))
-int f(string s) { return MACRO(s); })cc";
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests the corner case of the identity macro, specifically that it is
+// discarded in the rewrite rather than preserved (like PLUS is preserved in the
+// previous test).  This behavior is of dubious value (and marked with a FIXME
+// in the code), but we test it to verify (and demonstrate) how this case is
+// handled.
+TEST_F(TransformerTest, IdentityMacro) {
+  std::string Input = R"cc(
+#define ID(e) e
+int f(string s) { return ID(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define ID(e) e
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// No rewrite is applied when the changed text does not encompass the entirety
+// of the expanded text. That is, the edit would have to be applied to the
+// macro's definition to succeed and editing the expansion point would not
+// suffice.

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D64518#1589857 , @ymandel wrote:

> In D64518#1589768 , @ilya-biryukov 
> wrote:
>
> > In D64518#1588092 , @ymandel wrote:
> >
> > > This seems like a good candidate for configuration -- the user could then 
> > > choose which mode to run in.  But, I'm also open to just reporting these 
> > > conditions as errors.  It's already in a context that returns Expected, 
> > > so its no trouble; it's just a matter of choosing what we think is 
> > > "correct".
> >
> >
> > WRT to returning `Expected` vs `Optional`. Either seems fine and in the 
> > spirit of the library, depending on whether we want to produce more 
> > detailed errors. However, if we choose `Optional` let's stick to it, as 
> > practice shows switching from `Optional` to `Expected` correctly is almost 
> > impossible, as that requires a lot of attention to make sure all clients 
> > consume the errors (and given that it's an error case, tests often don't 
> > catch unconsumed errors).
> >  I would personally go with `Optional` here (meaning the client code would 
> > have to say something generic like `could not map from macro expansion to 
> > source code`). But up to you, not a strong preference.
>
>
> I think we might be talking about different things here. I meant that the 
> *calling* function, `translateEdits`, returns `Expected`, so it would be easy 
> to return an error when `makeValidRange` returns `None`.  I agree that 
> `makeValidRange` (or whatever we choose to call it) should stick with 
> `Optional` for simplicity (with the generic interpretation of `None` being 
> "could not map from macro expansion to source code").


Ah, great, we're on the same page then. LGTM!

>> WRT to which cases we choose to handle, I'd start with a minimal number of 
>> supported examples (covering full macro expansion, or inside a single 
>> argument) and gradually add other cases as we find use-cases. What are your 
>> thoughts on that?
> 
> I assume you mean which cases `makeValidRange` should handle (successfully)? 
> If so, that sounds good.

Yes, exactly.

> But, what do you think about how to handle failures of `makeValidRange` -- 
> ignore them silently (which is what we're doing now) or treat them as errors?

I think it depends on the use-case, e.g. if we try to produce a clang-tidy fix 
for some warning and can't produce a fix because `makeValidRange` failed, then 
not showing the fix (i.e. failing silently) seems fine.
OTOH, if we're building a refactoring tool that should find an replace all 
occurrences of a matcher and apply the transformation, failing silently is 
probably not a good option, we should possibly list the locations where the 
transformation failed (so that users can do manual changes to complete the 
refactoring).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D64518#1589768 , @ilya-biryukov 
wrote:

> In D64518#1588092 , @ymandel wrote:
>
> > This seems like a good candidate for configuration -- the user could then 
> > choose which mode to run in.  But, I'm also open to just reporting these 
> > conditions as errors.  It's already in a context that returns Expected, so 
> > its no trouble; it's just a matter of choosing what we think is "correct".
>
>
> WRT to returning `Expected` vs `Optional`. Either seems fine and in the 
> spirit of the library, depending on whether we want to produce more detailed 
> errors. However, if we choose `Optional` let's stick to it, as practice shows 
> switching from `Optional` to `Expected` correctly is almost impossible, as 
> that requires a lot of attention to make sure all clients consume the errors 
> (and given that it's an error case, tests often don't catch unconsumed 
> errors).
>  I would personally go with `Optional` here (meaning the client code would 
> have to say something generic like `could not map from macro expansion to 
> source code`). But up to you, not a strong preference.


I think we might be talking about different things here. I meant that the 
*calling* function, `translateEdits`, returns `Expected`, so it would be easy 
to return an error when `makeValidRange` returns `None`.  I agree that 
`makeValidRange` (or whatever we choose to call it) should stick with 
`Optional` for simplicity (with the generic interpretation of `None` being 
"could not map from macro expansion to source code").

> WRT to which cases we choose to handle, I'd start with a minimal number of 
> supported examples (covering full macro expansion, or inside a single 
> argument) and gradually add other cases as we find use-cases. What are your 
> thoughts on that?

I assume you mean which cases `makeValidRange` should handle (successfully)? If 
so, that sounds good.  But, what do you think about how to handle failures of 
`makeValidRange` -- ignore them silently (which is what we're doing now) or 
treat them as errors?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D64518#1588092 , @ymandel wrote:

> This seems like a good candidate for configuration -- the user could then 
> choose which mode to run in.  But, I'm also open to just reporting these 
> conditions as errors.  It's already in a context that returns Expected, so 
> its no trouble; it's just a matter of choosing what we think is "correct".


WRT to returning `Expected` vs `Optional`. Either seems fine and in the spirit 
of the library, depending on whether we want to produce more detailed errors. 
However, if we choose `Optional` let's stick to it, as practice shows switching 
from `Optional` to `Expected` correctly is almost impossible, as that requires 
a lot of attention to make sure all clients consume the errors (and given that 
it's an error case, tests often don't catch unconsumed errors).
I would personally go with `Optional` here (meaning the client code would have 
to say something generic like `could not map from macro expansion to source 
code`). But up to you, not a strong preference.

WRT to which cases we choose to handle, I'd start with a minimal number of 
supported examples (covering full macro expansion, or inside a single argument) 
and gradually add other cases as we find use-cases. What are your thoughts on 
that?




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional
+makeValidRange(CharSourceRange Range, const MatchResult ) {
+  const SourceManager  = *Result.SourceManager;

ymandel wrote:
> ilya-biryukov wrote:
> > Could we add unit tests for this particular function?
> > 
> > Interesting cases (`[[` and `]]` mark the start and end of a range):
> > ```
> > #define FOO(a) a+a;
> > #define BAR 10+
> > 
> > // change part of a macro argument
> > int a = FOO([[10]] + 10);
> > 
> > // change the whole macro expansion
> > int b = [[FOO(10+10)]];
> > 
> > // Try to change 10 inside 'BAR', but not '+'.
> > // Should this fail? Should we give a warning?
> > int c = BAR 3; 
> > 
> > // Try changing the lhs (10) of a binary expr, but not rhs.
> > // Is that allowed? Should we give a warning?
> > int d = FOO(10);
> > ```
> Sure. What do you think of exposing this function in 
> clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from 
> there?
Sounds reasonable. Was thinking of a better name, maybe something like 
`getRangeForEdit()`?
Would also suggest to accept `SourceManager` and `LangOptions` instead of 
`MatchResult` to narrow down the requirements on the clients.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 210148.
ymandel added a comment.

tweaks in response to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518

Files:
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -137,7 +137,7 @@
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@
   testRule(ruleStrlenSize(), Input, Input);
 }
 
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
-  std::string Input = R"cc(
-#define ID(e) e
-int f(string s) { return ID(strlen(s.c_str())); })cc";
-  std::string Expected = R"cc(
-#define ID(e) e
-int f(string s) { return ID(REPLACED); })cc";
-  testRule(ruleStrlenSize(), Input, Expected);
-}
-
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
@@ -619,23 +608,114 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
+  std::string Input = R"cc(
+#define ZERO 0
+int f(string s) { return ZERO; }
+  )cc";
+  std::string Expected = R"cc(
+#define ZERO 0
+int f(string s) { return 999; }
+  )cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+   change(node(zero), text("999")));
+  testRule(R, Input, Expected);
+}
+
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
-int f(string s) { return MACRO(s); })cc";
-  testRule(ruleStrlenSize(), Input, Input);
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define MACRO(str) strlen((str).c_str())
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten.
+TEST_F(TransformerTest, MacroArg) {
+  std::string Input = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(REPLACED); }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
 }
 
-// This test handles the corner case where a macro called within another macro
-// expands to matching code, but the matched code is an argument to the nested
-// macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
-// will get this wrong, and transform the code. This test verifies that no such
-// transformation occurs.
-TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+// Tests that expressions in macro arguments can be rewritten, even when the
+// macro call occurs inside another macro's definition.
+TEST_F(TransformerTest, MacroArgInMacroDef) {
   std::string Input = R"cc(
 #define NESTED(e) e
 #define MACRO(str) NESTED(strlen((str).c_str()))
-int f(string s) { return MACRO(s); })cc";
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests the corner case of the identity macro, specifically that it is
+// discarded in the rewrite rather than preserved (like PLUS is preserved in the
+// previous test).  This behavior is of dubious value (and marked with a FIXME
+// in the code), but we test it to verify (and demonstrate) how this case is
+// handled.
+TEST_F(TransformerTest, IdentityMacro) {
+  std::string Input = R"cc(
+#define ID(e) e
+int f(string s) { return ID(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define ID(e) e
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// No rewrite is applied when the changed text does not encompass the entirety
+// of the expanded text. That is, the edit would have to be applied to the
+// macro's definition to succeed and editing the expansion point would not
+// suffice.
+TEST_F(TransformerTest, NoPartialRewriteOMacroExpansion) 

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 2 inline comments as done.
ymandel added a comment.

In D64518#1585917 , @ilya-biryukov 
wrote:

> This clearly increases the utility of the library, but also seems to add 
> corner cases that the library won't handle (see the comment about unittests 
> for an example).
>  WDYT about those? Are they important, should we support producing warnings 
> in those cases to let the users know things might get broken?


That's a really good question.  The code explicitly chooses to treat these 
failures like "this didn't match" rather than "this matched and now there's an 
error".  That reflects the split that some users will want to know while others 
will want the system to always skip such matches, just like it skips 
non-matching expressions.

This seems like a good candidate for configuration -- the user could then 
choose which mode to run in.  But, I'm also open to just reporting these 
conditions as errors.  It's already in a context that returns Expected, so its 
no trouble; it's just a matter of choosing what we think is "correct".




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional
+makeValidRange(CharSourceRange Range, const MatchResult ) {
+  const SourceManager  = *Result.SourceManager;

ilya-biryukov wrote:
> Could we add unit tests for this particular function?
> 
> Interesting cases (`[[` and `]]` mark the start and end of a range):
> ```
> #define FOO(a) a+a;
> #define BAR 10+
> 
> // change part of a macro argument
> int a = FOO([[10]] + 10);
> 
> // change the whole macro expansion
> int b = [[FOO(10+10)]];
> 
> // Try to change 10 inside 'BAR', but not '+'.
> // Should this fail? Should we give a warning?
> int c = BAR 3; 
> 
> // Try changing the lhs (10) of a binary expr, but not rhs.
> // Is that allowed? Should we give a warning?
> int d = FOO(10);
> ```
Sure. What do you think of exposing this function in 
clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This clearly increases the utility of the library, but also seems to add corner 
cases that the library won't handle (see the comment about unittests for an 
example).
WDYT about those? Are they important, should we support producing warnings in 
those cases to let the users know things might get broken?




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional
+makeValidRange(CharSourceRange Range, const MatchResult ) {
+  const SourceManager  = *Result.SourceManager;

Could we add unit tests for this particular function?

Interesting cases (`[[` and `]]` mark the start and end of a range):
```
#define FOO(a) a+a;
#define BAR 10+

// change part of a macro argument
int a = FOO([[10]] + 10);

// change the whole macro expansion
int b = [[FOO(10+10)]];

// Try to change 10 inside 'BAR', but not '+'.
// Should this fail? Should we give a warning?
int c = BAR 3; 

// Try changing the lhs (10) of a binary expr, but not rhs.
// Is that allowed? Should we give a warning?
int d = FOO(10);
```



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:99
+
+  std::pair beginInfo = 
SM.getDecomposedLoc(Range.getBegin());
+  std::pair endInfo = SM.getDecomposedLoc(Range.getEnd());

naming NIT: use `BeginInfo`



Comment at: clang/unittests/Tooling/TransformerTest.cpp:625
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+   change(node(zero), text("0")));
+  testRule(R, Input, Expected);

could we change to something other than `0` to make sure it's not the macro 
being expanded?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518



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

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added a project: clang.

Currently, Transformer rejects any changes to source locations inside macro
expansions. This change relaxes that constraint to allow rewrites when the
entirety of the expansion is replaced, since that can be mapped to replacing the
entirety of the expansion range in the file source.  This change makes
Transformer consistent with the handling of edit ranges in `clang::edit::Commit`
(which is used, for example, for applying `FixItHint`s from diagnostics).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64518

Files:
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -137,7 +137,7 @@
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@
   testRule(ruleStrlenSize(), Input, Input);
 }
 
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
-  std::string Input = R"cc(
-#define ID(e) e
-int f(string s) { return ID(strlen(s.c_str())); })cc";
-  std::string Expected = R"cc(
-#define ID(e) e
-int f(string s) { return ID(REPLACED); })cc";
-  testRule(ruleStrlenSize(), Input, Expected);
-}
-
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
@@ -619,23 +608,114 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
+  std::string Input = R"cc(
+#define ZERO 0
+int f(string s) { return ZERO; }
+  )cc";
+  std::string Expected = R"cc(
+#define ZERO 0
+int f(string s) { return 0; }
+  )cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+   change(node(zero), text("0")));
+  testRule(R, Input, Expected);
+}
+
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
-int f(string s) { return MACRO(s); })cc";
-  testRule(ruleStrlenSize(), Input, Input);
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define MACRO(str) strlen((str).c_str())
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten.
+TEST_F(TransformerTest, MacroArg) {
+  std::string Input = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS(e) e + 1
+int f(string s) { return PLUS(REPLACED); }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
 }
 
-// This test handles the corner case where a macro called within another macro
-// expands to matching code, but the matched code is an argument to the nested
-// macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
-// will get this wrong, and transform the code. This test verifies that no such
-// transformation occurs.
-TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+// Tests that expressions in macro arguments can be rewritten, even when the
+// macro call occurs inside another macro's definition.
+TEST_F(TransformerTest, MacroArgInMacroDef) {
   std::string Input = R"cc(
 #define NESTED(e) e
 #define MACRO(str) NESTED(strlen((str).c_str()))
-int f(string s) { return MACRO(s); })cc";
+int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests the corner case of the identity macro, specifically that it is
+// discarded in the rewrite rather than preserved (like PLUS is preserved in the
+// previous test).  This behavior is of dubious value (and marked with a FIXME
+// in the code), but we test it to verify (and demonstrate) how this case is
+// handled.
+TEST_F(TransformerTest, IdentityMacro) {
+  std::string Input = R"cc(
+#define ID(e) e
+int f(string s) { return ID(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define ID(e) e
+int f(string s)