================
@@ -296,12 +296,16 @@ static bool fillRanges(MemoryBuffer *Code,
}
if (!EmptyLengths)
Length = Lengths[I];
+ if (Length == 0) {
+ errs() << "error: length should be at least 1\n";
+ return true;
+ }
if (Offset + Length > CodeSize) {
errs() << "error: invalid length " << Length << ", offset + length ("
- << Offset + Length << ") is outside the file.\n";
+ << Offset + Length << ") is outside the file\n";
return true;
}
- Ranges.push_back(tooling::Range(Offset, Length));
+ Ranges.push_back(tooling::Range(Offset, Length - 1));
----------------
kadircet wrote:
> I didn't fix it there because we would have to change all unit tests
> involving range formatting.
sorry I am not sure how to respond to that. Can you elaborate why updating
unittests, when we're supposed to be fixing a bug in a library used by many
applications was deemed undesired?
IIUC, what you're saying is, you also think the actual issue is in the `Format`
library, which is used by many other tools (clang-format, clangd, clang-tidy,
include-cleaner, and many many more). All of which use `Ranges` as format
library's contract declares, a 0-based offset and a length for number of bytes
starting from that offset, possibly `0`. clang-format tool used to accept
command line arguments exactly with the same contract, and now we're changing
semantics here, at the cost of breaking existing workflows, and diverging from
rest of the tools.
This discrepancy now requires ecosystem to use tools that embed format
differently (e.g. when an editor is making a range-formatting request, it needs
to provide different ranges if formatter is clangd vs clang-format ?). Moreover
all the tests in
[clang/unittests/Format/FormatTestSelective.cpp](https://github.com/llvm/llvm-project/blob/main/clang/unittests/Format/FormatTestSelective.cpp)
are telling a similar story (`length=0` is heavily used).
I don't understand how all of this is justified in order to not update some
unittests. Maybe we can figure out a way to make this change less intrusive for
unittests? There's a good chance we can both fix the issue, and keep the CLI
interface the same by defining some semantics around ranges with `length=0`.
https://github.com/llvm/llvm-project/pull/143302
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
