Retrieve BinaryOperator::getOpcode and BinaryOperator::getOpcodeStr via libclang and its python interface.
Details
Diff Detail
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 | ||
---|---|---|
1593 | 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 | What about this? How is it not exposed? This works in the C++ test apparently(?) | |
tools/libclang/CIndex.cpp | ||
6929 | I'd rename to getBinaryOpcode (note the casing, more consistent). Same below, rename to getBinaryOpcodeString (casing + full words) | |
6943 | 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 | 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 ↗ | (On Diff #229701) | This seems like just a clang-format change. Maybe we could separate these as a NFC commit? |
436 ↗ | (On Diff #229701) | This seems like just a clang-format change. Maybe we could separate these as a NFC commit? |
1369 ↗ | (On Diff #229701) | 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.
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...