Page MenuHomePhabricator

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

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

Details

Summary

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

Diff Detail

Event Timeline

RedX2501 updated this revision to Diff 28757.Jun 30 2015, 12:01 AM
RedX2501 retitled this revision from to Retrieve BinaryOperator::getOpcode and BinaryOperator::getOpcodeStr via libclang and its python interface.
RedX2501 updated this object.
RedX2501 edited the test plan for this revision. (Show Details)
RedX2501 added a subscriber: Unknown Object (MLST).

You also reviewed my other libclang patch, is it possible to also review this one?

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

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
6749

I'd rename to getBinaryOpcode (note the casing, more consistent).

Same below, rename to getBinaryOpcodeString (casing + full words)

6763

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

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?