Retrieve BinaryOperator::getOpcode and BinaryOperator::getOpcodeStr via libclang and its python interface.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 41085 Build 41249: arc lint + arc unit
Event Timeline
The code owners file says you are the owner of all parts that do not belong to somebody else. So i hope it is ok if i attach you as reviewer to this.
+milian - can you take a look at this patch (or do you know somebody who might be in a good position to review)
This looks good to me, but it's missing a unit test. Take a look at http://reviews.llvm.org/D13388 for how to do that in principle.
bindings/python/clang/cindex.py | ||
---|---|---|
1589 ↗ | (On Diff #39158) | TIL chaining comparisons in Python is OK... :) I first though this expression gives the wrong result and you need min <= val and val < max. But indeed your expression does exactly this... |
bindings/python/tests/cindex/test_cursor.py | ||
333 ↗ | (On Diff #39158) | What about this? How is it not exposed? This works in the C++ test apparently(?) |
tools/libclang/CIndex.cpp | ||
6749 ↗ | (On Diff #39158) | I'd rename to getBinaryOpcode (note the casing, more consistent). Same below, rename to getBinaryOpcodeString (casing + full words) |
6763 ↗ | (On Diff #39158) | I think this should have a enum CX_BinaryOperatorKind as parameter instead. There's BinaryOperator::getOpcode(Opcode) you can use. |
bindings/python/tests/cindex/test_cursor.py | ||
---|---|---|
333 ↗ | (On Diff #39158) | Yeah, I was surprised too. I have no idea why one is exposed and the other not... But i don't feel like investigating it as I don't need it and this operator is not used often. |
From my POV this is still fine. Manuel, Sergey - could you have a look at this please and push it upstream if you agree with me?
Thanks
For CX_BinaryOperatorKind please change the enumerators to have an explicit integer assignment; this is to emphasize that they are API contract and should not change with subsequent changes (someone may inadvertently add a new enumerator in the middle which will implicitly change all the subsequent enumerators).
Otherwise LGTM.
Where can i see the test reports? I'm having trouble running the test on my side on windows (VS2013).
I did run the python tests before commiting and those ran fine.
Tests need to be updated, for example:
clang/test/Index/remap-load.c:7:11: error: expected string not found in input
// CHECK: remap-load.c:2:10: BinaryOperator= Extent=[2:10 - 2:23]
^
<stdin>:334:1: note: scanning from here
CHECK: remap-load.c:2:10: BinaryOperator=+ Extent=[2:10 - 2:23]
^
<stdin>:334:11: note: possible intended match here
CHECK: remap-load.c:2:10: BinaryOperator=+ Extent=[2:10 - 2:23]
^
the failing tests are:
Failing Tests (9):
Clang :: Index/blocks.c Clang :: Index/c-index-api-loadTU-test.m Clang :: Index/nested-binaryoperators.cpp Clang :: Index/preamble.c Clang :: Index/print-type.c Clang :: Index/print-type.cpp Clang :: Index/recursive-cxx-member-calls.cpp Clang :: Index/remap-load.c Clang :: Index/usrs.m
Another ping :-). This looks like a very useful addition (and I need it myself so I'll end up applying this patchset manually) and I would like to find it merged. It doesn't look it brings any kind of negative value to the C API.
Ping. It would indeed be very helpful to have information about the kind of an operator :-)
Would it be beneficial for this to be just called getOpcode? If it was, it could also be applied to UnaryOperator::getOpcode.
I have been using this revision locally for 9 months in a system that uses libclang to parse C code and extract information. I'm commandeering this issue since the original author (@RedX2501) has not posted on this issue since 2016 and I'm willing to make an attempt to get this merged. I have updated the revision to apply clearly on LLVM master (f897d087d09dbbccec3417f812109ed534b94248) and I have updated and run all the tests.
I am happy to make other changes if needed to make this revision mergeable. Just tell me what's needed.
Hi @arthurp, I can review the libclang part of the patch.
Could you please remove the changes that are just code formatting? You can land those as a separate NFC commit.
clang/tools/libclang/CIndex.cpp | ||
---|---|---|
259 | This seems like just a clang-format change. Maybe we could separate these as a NFC commit? | |
436 | This seems like just a clang-format change. Maybe we could separate these as a NFC commit? | |
1369 | This seems like just a clang-format change. Maybe we could separate these as a NFC commit? Could you please leave out all such changes from this patch? It would be easier to review. (It seems to me a bunch of changes below are of this nature.) |
What about unary operations? I don't see those supported in the current version of libclang Python either.
Edit: for anyone who's interested, I've forked LLVM/Clang to add support for getting binary and unary operators using libclang. I would submit the patch to this thread, but I am not sure how to update the test cases for the unary operator. See https://github.com/calebh/llvm-project/tree/release/9.x for the fork (release/9.x branch)
Any news? It's amazing that such a basic feature (getting an operator kind) with working patches (since 2015) is not yet available in 2020. If there is any work that needs to be done, I propose myself to do it.
This seems like just a clang-format change. Maybe we could separate these as a NFC commit?