ymandel marked 2 inline comments as done.
ymandel added a comment.
In D64518#1585917 <https://reviews.llvm.org/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<CharSourceRange>
+makeValidRange(CharSourceRange Range, const MatchResult &Result) {
+ const SourceManager &SM = *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
