[clang] [libclang/python] Fix bug in `SourceRange.__contains__`, add tests (PR #101802)

Vlad Serebrennikov via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 9 11:08:20 PDT 2024


================
@@ -386,6 +386,10 @@ def __contains__(self, other):
         # same file, in between lines
         if self.start.line < other.line < self.end.line:
             return True
+        # between columns in one-liner range
+        elif self.start.line == other.line == self.end.line:
----------------
Endilll wrote:

> we already have SourceLocation.__eq__ implemented (which just passes directly to the C interface)

I didn't realize that, thank you!

> and that one is file-sensitive. I.e. loc1 == loc2 gives false if their line & column are the same but their file name is different, while loc1 <= loc2 is True in that case.

This is sure ugly.

I see 4 ways forward:
1) Carry on with implementing `__contains__` in Python in terms of line and column numbers. I don't like that precisely because of its file-insensitivity.
2) Do what `clang_equalLocations` does, i.e. add a libclang function that reimplements `SourceLocation::operator<` in terms of `CXSourceLocation.int_data` (which corresponds to `SourceLocation::ID` the original `operator<` is implemented in terms of).
3) Do what `clang_equalLocations` does, but in Python, i.e. implement `SourceLocation.__contains__` in terms of `SourceLocation.int_data`.
4) Literally expose `SourceLocation::operator<` via libclang as I suggested earlier.

I don't like option 1, because we'd intentionally provide a broken API. I cautiously support option 3 as having minimal impact on various interfaces, but I'm perplexed that libclang interprets guts of `SourceLocation` (which in turn is an opaque value that only `SourceManager` can make sense of).

Now I wonder how well libclang is tested w.r.t. potential changes in the underlying C++ API. If I were to change something in `SourceLocation` layout, libclang would be the last thing on my mind, even though it's our stable API (and ABI, I believe).

CC @AaronBallman I wonder what your thoughts are

https://github.com/llvm/llvm-project/pull/101802


More information about the cfe-commits mailing list