[PATCH] D107503: [clang][Rewriter] patch to fix bug with ReplaceText erasing too many characters when text has been inserted before the replace location.

Tue Aug 17 16:23:22 PDT 2021

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