jdenny added a comment.
Thanks for working on this. I agree there's a bug. However, this patch
doesn't fix the bug. It provides a workaround.
That is, the current default `ReplaceText` behavior, as revealed in the first
test added by this patch, doesn't make sense.
`IncludeInsertsAtBeginOfRange=true` (the default) should mean to also remove
the text that was previously inserted before the specified range. Instead, the
previously inserted text confuses the implementation, so it removes extra text
//after// the specified range. It's hard to imagine anyone is relying on that
broken behavior.
I see a couple of potential paths forward. Either:
1. Change the patch summary and the new comments to make it clear that the
default behavior is broken, so specifying `IncludeInsertsAtBeginOfRange=true`
is the only way to ensure reasonable behavior. `FIXME:` should prefix some of
those comments.
2. Fix the bug. I've added an inline comment to suggest a possible fix, but I
haven't tested it carefully.
Either way, `RemoveText` should be addressed in the same manner as it has the
same bug.
================
Comment at: clang/lib/Rewrite/Rewriter.cpp:132
StringRef NewStr) {
unsigned RealOffset = getMappedOffset(OrigOffset, true);
Buffer.erase(RealOffset, OrigLength);
----------------
I think this might fix the bug, but I haven't tested thoroughly.
One problem is ensuring backward compatibility here and in callers while
keeping a consistent interface. That is, the previous default here was
`IncludeInsertsAtBeginOfRange=false`, but it seems the intended default
elsewhere is meant to be the opposite.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107503/new/
https://reviews.llvm.org/D107503
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
