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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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:
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?
clang-tidy: warning: #includes are not sorted properly [llvm-include-order]
not useful