Page MenuHomePhabricator

Retrieve BinaryOperator::getOpcode and BinaryOperator::getOpcodeStr via libclang and its python interface
AcceptedPublic

Authored by arthurp on Jun 30 2015, 12:01 AM.

Details

Summary

Retrieve BinaryOperator::getOpcode and BinaryOperator::getOpcodeStr via libclang and its python interface.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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)

milianw edited edge metadata.Oct 13 2015, 5:03 AM

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.

RedX2501 updated this revision to Diff 39158.Nov 3 2015, 8:50 PM

Added module tests to libclang and python.

kfunk added inline comments.Nov 4 2015, 12:57 AM
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.

RedX2501 marked 2 inline comments as done.Nov 4 2015, 2:35 AM
RedX2501 added inline comments.
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.

RedX2501 updated this revision to Diff 39176.Nov 4 2015, 2:37 AM

Changed points raised during review.

mpuels added a subscriber: mpuels.Nov 22 2015, 9:42 AM
milianw accepted this revision.Nov 22 2015, 10:55 AM
milianw edited edge metadata.

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

This revision is now accepted and ready to land.Nov 22 2015, 10:55 AM
kfunk accepted this revision.Nov 24 2015, 12:35 PM
kfunk edited edge metadata.

Looks good to me now. I'd also appreciate another +1 from somone else, though.

akyrtzi edited edge metadata.Dec 8 2015, 11:46 AM
akyrtzi added a subscriber: akyrtzi.

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.

RedX2501 updated this revision to Diff 42267.Dec 8 2015, 10:20 PM
RedX2501 edited edge metadata.

Added explicit integer assignment to enum to emphasize api contract behaviour.

The patch causes tests to fail, please look into it.

Also you need to add the functions in libclang.exports.

RedX2501 added a comment.EditedDec 9 2015, 9:01 PM

The patch causes tests to fail, please look into it.

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.

RedX2501 updated this revision to Diff 42386.Dec 9 2015, 9:56 PM

Added functions to libclang.exports.

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
csarn added a subscriber: csarn.Feb 4 2016, 5:40 AM
RedX2501 updated this revision to Diff 52204.Mar 31 2016, 7:37 AM

This should fix all the breaking tests.

RedX2501 updated this revision to Diff 52519.Apr 3 2016, 7:01 PM

Fixed warnings when compiling c-index-test about too long lines.

Has this been merged yet?

milianw set the repository for this revision to rC Clang.Jan 9 2018, 4:47 AM

still looks good to me. can someone else please review and commit this?

still looks good to me. can someone else please review and commit this?

Any updates on this? This feature will be very helpful. :)

still looks good to me. can someone else please review and commit this?

Ping

still looks good to me. can someone else please review and commit this?

Ping

jbcoe added a reviewer: jbcoe.Jun 28 2018, 2:01 AM

still looks good to me. can someone else please review and commit this?

Ping

Any updates on this? :)

Burgos added a subscriber: Burgos.Nov 12 2018, 2:51 PM

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 :-)

dyhe83 added a subscriber: dyhe83.Feb 11 2019, 7:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 7:27 PM

still looks good to me. can someone else please review and commit this?

ping

still looks good to me. can someone else please review and commit this?

ping

please

still looks good to me. can someone else please review and commit this?

ping

please

any updates on this?

Ping. It would indeed be very helpful to have information about the kind of an operator :-)

Any updates on this?

Would it be beneficial for this to be just called getOpcode? If it was, it could also be applied to UnaryOperator::getOpcode.

arthurp commandeered this revision.Nov 16 2019, 11:25 AM
arthurp added a reviewer: RedX2501.

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.

arthurp updated this revision to Diff 229701.Nov 16 2019, 11:27 AM

Add support for the new <=> (cmp) operator. Rebase revision against master.

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.)

jbcoe resigned from this revision.Jan 8 2020, 8:55 AM
calebh added a subscriber: calebh.EditedFeb 2 2020, 7:25 PM

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)