Page MenuHomePhabricator

[libTooling] Change Transformer's `cat` to handle some cases of text in macros.
ClosedPublic

Authored by ymandel on Jun 18 2020, 1:56 PM.

Details

Summary

Currently, cat validates range selections before extracting the corresponding
source text. However, this means that any range inside a macro is rejected as an
error. This patch changes the implementation to first try to map the range to
something reasonable. This makes the behavior consistent with handling of ranges
used for selecting portions of the source to edit.

Diff Detail

Event Timeline

ymandel created this revision.Jun 18 2020, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2020, 1:56 PM
tdl-g added a comment.Jun 19 2020, 8:32 AM

LGTM. I found the change description confusing, since it talks about the selection() stencil but the code is all about the cat() stencil. I realize (now) that the former is deprecated in favor of the latter. But the change description is still confusing.

tdl-g accepted this revision.Jun 19 2020, 8:33 AM
This revision is now accepted and ready to land.Jun 19 2020, 8:33 AM
ymandel retitled this revision from [libTooling] Change `selection` stencil to handle some cases of text in macros. to [libTooling] Change Transformer's `cat` to handle some cases of text in macros..Jun 19 2020, 8:43 AM
ymandel edited the summary of this revision. (Show Details)

LGTM. I found the change description confusing, since it talks about the selection() stencil but the code is all about the cat() stencil. I realize (now) that the former is deprecated in favor of the latter. But the change description is still confusing.

Whoops, I'd forgotten that we'd deprecated that combinator. Thanks for catching that. I've updated the description correspondingly.

Tests show that this breaks the test for the clang tidy abseil-string-find-str-contains. Curious if this is a desirable change in behavior (in which case I'll update your test file) or the wrong behavior:

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252

void no_macros() {
   std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
 }

Interesting, in all three of those cases, it's reasonable to replace the entire expression, thus eliminating the macro. None of those "tear" the macro; if we had a case like

#define FOO(a,b,c,d) ((a).find(b) == std::string::npos ? (c) : (d))
FOO("helo", "x", 5, 6);

I guess in that case we'd want to suppress an edit change, since it would have to modify the macro to make the change. But I guess all the existing test cases are actually safe to convert with the macro.

Do you want to remove the existing macro cases and add the "tearing" one above to confirm that it doesn't propose an edit?

Tests show that this breaks the test for the clang tidy abseil-string-find-str-contains. Curious if this is a desirable change in behavior (in which case I'll update your test file) or the wrong behavior:

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252

void no_macros() {
   std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
 }
gribozavr2 accepted this revision.Jun 19 2020, 10:16 AM
ymandel updated this revision to Diff 272150.Jun 19 2020, 11:40 AM

Fixed clang tidy lit test.

Interesting, in all three of those cases, it's reasonable to replace the entire expression, thus eliminating the macro. None of those "tear" the macro; if we had a case like

#define FOO(a,b,c,d) ((a).find(b) == std::string::npos ? (c) : (d))
FOO("helo", "x", 5, 6);

I guess in that case we'd want to suppress an edit change, since it would have to modify the macro to make the change. But I guess all the existing test cases are actually safe to convert with the macro.

Do you want to remove the existing macro cases and add the "tearing" one above to confirm that it doesn't propose an edit?

Done.

Tests show that this breaks the test for the clang tidy abseil-string-find-str-contains. Curious if this is a desirable change in behavior (in which case I'll update your test file) or the wrong behavior:

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252

void no_macros() {
   std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
 }
This revision was automatically updated to reflect the committed changes.