Page MenuHomePhabricator

Expose cxx constructor and method properties through libclang and python bindings.
ClosedPublic

Authored by jbcoe on Dec 11 2015, 2:31 PM.

Details

Summary

I have exposed the following function through libclang and the clang.cindex python bindings:

clang_CXXConstructor_isConvertingConstructor,
clang_CXXConstructor_isCopyConstructor,
clang_CXXConstructor_isDefaultConstructor,
clang_CXXConstructor_isMoveConstructor,
clang_CXXMethod_isDefaulted

I need (some of) these methods for a C++ code model I am building in Python to drive a code generator.

Diff Detail

Repository
rL LLVM

Event Timeline

jbcoe updated this revision to Diff 42584.Dec 11 2015, 2:31 PM
jbcoe retitled this revision from to Expose cxx constructor and method properties through libclang and python bindings..
jbcoe updated this object.
jbcoe added reviewers: compnerd, akyrtzi, skalinichev, eliben.
compnerd added inline comments.Dec 12 2015, 12:38 AM
bindings/python/tests/cindex/test_cursor.py
137 ↗(On Diff #42584)

How about adding a move constructor and verifying that we don't detect it as a copy constructor?

tools/c-index-test/c-index-test.c
778 ↗(On Diff #42584)

Missing space after the comma.

780 ↗(On Diff #42584)

Similar

compnerd edited edge metadata.Dec 12 2015, 12:39 AM

Also, it may be worthwhile to only expose what is actually needed. We can expand to the other interfaces when there is an actual need for it.

jbcoe updated this revision to Diff 42637.Dec 12 2015, 2:54 AM
jbcoe updated this object.
jbcoe edited edge metadata.

Added test as recommended in review.

jbcoe marked an inline comment as done.Dec 12 2015, 2:57 AM

I have possible need for all of the methods exposed and definite need for is_deleted.

I had thought that exposing more things through lbclang would be better, what's the motivation for exposing less?

The interfaces are part of the C API, which we try to keep stable, so minimizing the exposure is ideal. However, if there is a need for it, we can absolutely add the new interfaces.

Sorry, didn't catch the new test case yesterday. Really if you have a need for these interfaces, with the new test, I think that this should be fine.

test/Index/load-classes.cpp
31 ↗(On Diff #42637)

Can you also add a test here for:

class c {
    explicit c(const c &);
};

This should not be reported as a converting constructor, but should be reported as a copy constructor.

jbcoe added a comment.Dec 12 2015, 9:34 AM

I've added (locally)

class c {
    explicit c(const c &);
};

it is reported as (copy constructor) (explicit converting constructor). I think this is correct but I'm not sure if we want it to be declared as such.

I'm tempted to remove the implicit from (implicit converting constructor) and remove the '(explicit converting constructor)` altogether as I don't need it and it feels like noise rather than useful information.

skalinichev added inline comments.Dec 13 2015, 4:29 AM
test/Index/index-file.cpp
47 ↗(On Diff #42637)

Shouldn't it be (default constructor) (defaulted) instead?

jbcoe added inline comments.Dec 13 2015, 10:18 AM
test/Index/index-file.cpp
47 ↗(On Diff #42637)

It should. (default constructor) is currently being swallowed by the glob.

jbcoe updated this revision to Diff 42670.EditedDec 13 2015, 10:28 AM

Minor additions to index tests following review

jbcoe marked 3 inline comments as done.Dec 13 2015, 10:29 AM

I'm tempted to remove the implicit from (implicit converting constructor) and remove the '(explicit converting constructor)` altogether as I don't need it and it feels like noise rather than useful information.

The printing here is for the test binary, its not what is reported to the user, and can be inferred from the information. The noise here ensures that the matching is what is supposed to be part of the interface contract. We could remove the AllowExplicit parameter to clang_CXXConstructor_isConvertingConstructor, and let the user determine via a clang_CXXConstructor_isExplicit. Im slightly leaning towards letting the user determine it themselves.

jbcoe updated this revision to Diff 42672.Dec 13 2015, 3:02 PM

Fix formatting.

jbcoe marked 2 inline comments as done.Dec 13 2015, 3:10 PM

I've made all the suggested edits.

From my reading of the C++14 draft standard, a converting constructor is not explicit so defaulting the AllowExplicit flag to true seems inconsistent.

I'm happy to add clang_CXXConstructorIsExplicit if you think that's the best way to go.

Yeah, that is why Im not strongly convinced its the right thing to do. I think that we could filter to the ones that are not marked explicit. According to C++11:

12.3.1: A constructor declared without the function-specifier explicit specifies a conversion from the types of its parameters to the type of its class. Such a constructor is called a converting constructor.

So, really, the additional flag seems incorrect to include.

jbcoe updated this revision to Diff 42784.Dec 14 2015, 2:48 PM

consider_explicit defaults to False in Python bindings for is_converting_constructor.

Please let me know if you'd like further changes. I can do all the code-generation I need with the current changes.

Lets go ahead and remove the parameter and not consider the explicit ones for the time being then.

jbcoe added a comment.EditedDec 14 2015, 4:28 PM

Do we want it removing from Python bindings, defaulting in Python bindings (as I've done) or removing from the libclang bindings?

he latter seems cleaner but potentially an issue as those bindings are meant to be stable. I suppose someone could later add another function to access explicit converting constructors if they needed it.

Exactly, I think that we can add another interface if needed in the future.

jbcoe updated this revision to Diff 43035.Dec 16 2015, 11:01 AM

Remove ConsiderExplicit flag from CXXMethod_isConvertingConstructor.

I was wondering if further changes are needed for this PR?

skalinichev edited edge metadata.Mar 8 2016, 1:47 AM

Generally looks pretty good to me. Maybe it's worth to factor out duplicating code, but I guess it's not so important.

Also the index-file.cpp test failing for me:
test/Index/index-file.cpp:57:11: error: expected string not found in input
// CHECK: [indexDeclaration]: kind: c++-instance-method | name: foo | {{.*}} (deleted) | loc: 36:8

@jbcoe, can you take a look?

compnerd accepted this revision.Mar 8 2016, 3:47 PM
compnerd edited edge metadata.

Sorry about the delay with this change. Thanks for removing the parameter. I think that this is fine as is. Do you have commit rights, or should I commit this on your behalf?

This revision is now accepted and ready to land.Mar 8 2016, 3:47 PM
jbcoe added a comment.Mar 10 2016, 4:30 PM

@skalinichev I get the same failure as you. This patch is old now, all tests used to pass. Can you see what is wrong with the test logic?

jbcoe added a comment.Mar 10 2016, 4:46 PM

I don't know if it's at all helpful but I get the following output from llvm-lit on the failing test file. There's no mention of a function foo at all:

[startedTranslationUnit]
[enteredMainFile]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp
[indexDeclaration]: kind: type-alias | name: MyTypeAlias | USR: c:@MyTypeAlias | lang: C++ | cursor: TypeAliasDecl=MyTypeAlias:1:7 (Definition) | loc: 1:7 | semantic-container: [TU]
[indexDeclaration]: kind: function-template | name: Allocate | USR: c:@FT@>1#TAllocate | lang: C++ | cursor: FunctionDecl=Allocate:4:28 (Definition) | loc: 4:28 | semantic-container
[indexDeclaration]: kind: namespace | name: rdar14063074 | USR: c:@N@rdar14063074 | lang: C++ | cursor: Namespace=rdar14063074:8:11 (Definition) | loc: 8:11 | semantic-container: [T
[indexDeclaration]: kind: c++-class-template | name: TS | USR: c:@N@rdar14063074@ST>1#T@TS | lang: C++ | cursor: StructDecl=TS:10:8 (Definition) | loc: 10:8 | semantic-container: [r
[indexDeclaration]: kind: struct-template-spec | name: TS | USR: c:@N@rdar14063074@S@TS>#I | lang: C++ | cursor: StructDecl=TS:11:8 (Definition) [Specialization of TS:10:8] | loc: 1
[indexDeclaration]: kind: function-template | name: tfoo | USR: c:@N@rdar14063074@FT@>1#Ttfoo#v# | lang: C++ | cursor: FunctionDecl=tfoo:14:6 (Definition) | loc: 14:6 | semantic-con
[indexDeclaration]: kind: function-template-spec | name: tfoo | USR: c:@N@rdar14063074@F@tfoo<#I># | lang: C++ | cursor: FunctionDecl=tfoo:15:6 (Definition) [Specialization of tfoo:
[indexDeclaration]: kind: namespace | name: crash1 | USR: c:@N@crash1 | lang: C++ | cursor: Namespace=crash1:18:11 (Definition) | loc: 18:11 | semantic-container: [TU] | lexical-con
[indexDeclaration]: kind: c++-class-template | name: A | USR: c:@N@crash1@ST>1#T@A | lang: C++ | cursor: ClassDecl=A:19:28 (Definition) | loc: 19:28 | semantic-container: [crash1:18
[indexDeclaration]: kind: c++-instance-method | name: meth | USR: c:@N@crash1@ST>1#T@A@F@meth# | lang: C++ | cursor: CXXMethod=meth:21:8 | loc: 21:8 | semantic-container: [A:19:28] 
[indexDeclaration]: kind: c++-instance-method | name: meth | USR: c:@N@crash1@S@A>#I@F@meth# | lang: C++ | cursor: CXXMethod=meth:23:26 [Specialization of meth:21:8] | loc: 23:26 | 
[indexEntityReference]: kind: c++-class-template | name: A | USR: c:@N@crash1@ST>1#T@A | lang: C++ | cursor: TemplateRef=A:19:28 | loc: 23:18 | <parent>:: kind: c++-instance-method 
[indexDeclaration]: kind: c++-class | name: B | USR: c:@S@B | lang: C++ | cursor: ClassDecl=B:27:7 (Definition) | loc: 27:7 | semantic-container: [TU] | lexical-container: [TU] | is
[indexDeclaration]: kind: field | name: x_ | USR: c:@S@B@FI@x_ | lang: C++ | cursor: FieldDecl=x_:28:15 (Definition) (mutable) | loc: 28:15 | semantic-container: [B:27:7] | lexical-
[indexDeclaration]: kind: field | name: y_ | USR: c:@S@B@FI@y_ | lang: C++ | cursor: FieldDecl=y_:29:7 (Definition) | loc: 29:7 | semantic-container: [B:27:7] | lexical-container: [
[indexDeclaration]: kind: constructor | name: B | USR: c:@S@B@F@B# | lang: C++ | cursor: CXXConstructor=B:31:3 (default constructor) (defaulted) | loc: 31:3 | semantic-container: [B
[indexDeclaration]: kind: constructor | name: B | USR: c:@S@B@F@B#I# | lang: C++ | cursor: CXXConstructor=B:32:3 (converting constructor) | loc: 32:3 | semantic-container: [B:27:7] 
[indexDeclaration]: kind: constructor | name: B | USR: c:@S@B@F@B#d# | lang: C++ | cursor: CXXConstructor=B:33:12 | loc: 33:12 | semantic-container: [B:27:7] | lexical-container: [B
[indexDeclaration]: kind: constructor | name: B | USR: c:@S@B@F@B#&1$@S@B# | lang: C++ | cursor: CXXConstructor=B:34:3 (copy constructor) (converting constructor) | loc: 34:3 | sema
[indexEntityReference]: kind: c++-class | name: B | USR: c:@S@B | lang: C++ | cursor: TypeRef=class B:27:7 | loc: 34:11 | <parent>:: kind: constructor | name: B | USR: c:@S@B@F@B#&1
[indexDeclaration]: kind: constructor | name: B | USR: c:@S@B@F@B#&&$@S@B# | lang: C++ | cursor: CXXConstructor=B:35:3 (move constructor) (converting constructor) | loc: 35:3 | sema
[indexEntityReference]: kind: c++-class | name: B | USR: c:@S@B | lang: C++ | cursor: TypeRef=class B:27:7 | loc: 35:5 | <parent>:: kind: constructor | name: B | USR: c:@S@B@F@B#&&$
[indexDeclaration]: kind: c++-class | name: C | USR: c:@S@C | lang: C++ | cursor: ClassDecl=C:39:7 (Definition) | loc: 39:7 | semantic-container: [TU] | lexical-container: [TU] | is
[indexDeclaration]: kind: constructor | name: C | USR: c:@S@C@F@C#&1$@S@C# | lang: C++ | cursor: CXXConstructor=C:40:12 (copy constructor) | loc: 40:12 | semantic-container: [C:39:7
[indexEntityReference]: kind: c++-class | name: C | USR: c:@S@C | lang: C++ | cursor: TypeRef=class C:39:7 | loc: 40:20 | <parent>:: kind: constructor | name: C | USR: c:@S@C@F@C#&1
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:1:21: warning: alias declarations are a C++11 extension [-Wc++11-extensions]
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:4:28: error: C++ requires a type specifier for all declarations
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:4:3: error: templates must have C++ linkage
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:11:8: error: template specialization requires 'template<>'
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:15:6: error: template specialization requires 'template<>'
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:20:12: warning: deleted function definitions are a C++11 extension [-Wc++11-extensions]
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:31:9: warning: defaulted function definitions are a C++11 extension [-Wc++11-extensions]
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:35:6: warning: rvalue references are a C++11 extension [-Wc++11-extensions]
[diagnostic]: /Users/jon/DEV/llvm-git-svn/tools/clang/test/Index/index-file.cpp:36:16: warning: deleted function definitions are a C++11 extension [-Wc++11-extensions]
jbcoe added a comment.Mar 17 2016, 3:37 AM

If I remove = delete from the declaration of the member function foo in test/Index/index-file.cpp line 36 then foo is reported in the output as

[indexDeclaration]: kind: c++-instance-method | name: foo | USR: c:@S@B@F@foo# | lang: C++ | cursor: CXXMethod=foo:36:8 | loc: 36:8 | semantic-container: [B:27:7] | lexical-container: [B:27:7] | isRedecl: 0 | isDef: 0 | isContainer: 0 | isImplicit: 0

with =delete in place, foo is not reported at all.

This is a behaviour change since I wrote this patch. I'm not sure what the correct behaviour should be. I can update the test and not check for the deleted function foo when we are confident that behaviour is correct.

The changed behaviour (not reporting deleted functions) is not part of my patch.

I see. There were some changes recently in the indexing functionality. I'm not sure whether this change is intended or not, but since it's not your fault and we already have a lot of tests confirming that clang_CXXMethod_isDeleted is working as expected (e.g. c-index-test -test-print-type) I think it's ok to remove this part of the test then

But just in case please open a bug report about skipped deleted methods with clangIndex.

tools/libclang/CIndex.cpp
7124 ↗(On Diff #43035)

Just occurred to me: what about deleted "not member" functions? Maybe clang_CXXMethod_isDeleted should be renamed to something like clang_Cursor_isDeleted and there we can use FunctionDecl::isDeleted()

jbcoe planned changes to this revision.Apr 4 2016, 7:14 AM

I plan to strip out the isDeleted methods as the associated tests now fail (which I need to raise a bug for) and need reworking to apply to non-member functions.

I'll submit a new patch for isDeleted on its own. The rest of this patch seems uncontentious.

I hope to find time this week to make the changes.

jbcoe updated this revision to Diff 53316.Apr 11 2016, 2:11 PM

Remove isDeleted as it needs to be added for non-member functions.

I will add isDeleted as an isolated follow up patch.

This revision is now accepted and ready to land.Apr 11 2016, 2:11 PM
jbcoe requested a review of this revision.Apr 11 2016, 2:11 PM
jbcoe edited edge metadata.
compnerd accepted this revision.Apr 11 2016, 8:07 PM
compnerd edited edge metadata.
This revision is now accepted and ready to land.Apr 11 2016, 8:07 PM
skalinichev accepted this revision.Apr 12 2016, 11:41 AM
skalinichev edited edge metadata.

LGTM!

bindings/python/tests/cindex/test_cursor.py
197 ↗(On Diff #53316)

You should remove it too.

jbcoe updated this object.Apr 13 2016, 12:24 AM
jbcoe edited edge metadata.
jbcoe updated this revision to Diff 53519.Apr 13 2016, 12:29 AM

Remove python test for deleted method.

I missed this because the python tests are not run as part of check-clang - is this something I could submit a patch to change?

jbcoe requested a review of this revision.Apr 13 2016, 12:30 AM
jbcoe edited edge metadata.
jbcoe marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
mardy added a subscriber: mardy.Mar 31 2020, 8:11 AM
mardy added a comment.Mar 31 2020, 8:14 AM

I'll submit a new patch for isDeleted on its own. The rest of this patch seems uncontentious.

I hope to find time this week to make the changes.

Was the patch for isDeleted ever submitted? If not, I can try to get my hands dirty with it :-)