This is an archive of the discontinued LLVM Phabricator instance.

[LIbClang] Report the named type for ElaboratedType
ClosedPublic

Authored by skalinichev on Aug 6 2015, 3:23 AM.

Diff Detail

Event Timeline

skalinichev updated this revision to Diff 31435.Aug 6 2015, 3:23 AM
skalinichev retitled this revision from to [libclang] Expose the ElaboratedType.
skalinichev updated this object.
skalinichev added a reviewer: akyrtzi.
skalinichev updated this object.Aug 7 2015, 2:08 AM
skalinichev added a subscriber: cfe-commits.
skalinichev updated this revision to Diff 31503.Aug 7 2015, 2:15 AM

Fixed minor formatting issues.

modocache added inline comments.
bindings/python/clang/cindex.py
1875 ↗(On Diff #31503)

Awesome, thanks for updating the Python bindings as well!

As with the C function, I believe this is a typo--shouldn't it be getElaboratedTypeUnderlyingType?

Furthermore, the convention in Python (and in this file) is to use snake_case, so this should probably be get_elaborated_type_underlying_type.

include/clang-c/Index.h
2859 ↗(On Diff #31503)

Tangential to this diff, but is there a reason these CXTypeKind enum values are undocumented? I can see how many would be familiar to most users, but I don't think "elaborated" is something that many users would immediately recognize. Should these be documented?

3224 ↗(On Diff #31503)

nit: Other \brief documentation in this file uses periods at the end of sentences. For example, clang_getArraySize, the function immediately prior to this newly added function, has the brief -\brief Return the array size of a constant array.--notice the trailing period.

3226 ↗(On Diff #31503)

nit: Rather than reference the CXType_Invalid type directly, most of the other documentation in this file states "an invalid type is returned."

3228 ↗(On Diff #31503)

I believe this is a typo--shouldn't it be clang_getElaboratedTypeUnderlyingType?

tools/libclang/CXType.cpp
987

The documentation of getNamedType() itself states "Retrieve the type named by the qualified-id.". As such, would it make sense to call this function clang_getNamedTypeUnderlyingElaboratedType, instead of clang_getElaboratedTypeUnderlyingType?

skalinichev updated this revision to Diff 31576.Aug 8 2015, 3:42 AM
skalinichev marked 6 inline comments as done.

Address raised issues.

modocache, great review btw, thanks!

modocache accepted this revision.Aug 9 2015, 9:32 PM
modocache added a reviewer: modocache.

Looks good to me! I don't have commit access, though, so you'll probably have to find a committer to review/commit for you, @skalinichev.

This revision is now accepted and ready to land.Aug 9 2015, 9:32 PM

Ping? Can someone commit it?

skalinichev updated this revision to Diff 33787.Sep 2 2015, 2:31 AM
skalinichev retitled this revision from [libclang] Expose the ElaboratedType to [LIbClang] Report the named type for ElaboratedType.
skalinichev updated this object.
skalinichev edited edge metadata.

It just occured to me, why do we need to expose the ElaboratedType if we can retrieve the original type and return it to user. Especially when we already do that for Attributed and Decayed types. So here is a much simplier solution.

klimek added a subscriber: klimek.Sep 3 2015, 9:27 AM

Is this still ready for submission or do you want to change something (not clear after your last comment :)

skalinichev updated this revision to Diff 34019.Sep 4 2015, 2:11 AM

Rebased.

Yes, now it's ready.

klimek added a comment.Sep 7 2015, 7:55 AM

Ok, so looking over the new patch:
It seems like attributed types and decayed types are somewhat different from elaborated types:

  • decayed types don't lose any information if we look through to the original type
  • attributed types lose only information that can be considered an implementation detail
  • losing the nested name specifiers a user wrote can be considered part of the name (the lookup will be different)

I'm happy to be convinced that this doesn't matter here, and that inconsistencies like in the changed tests (canonical type: elaborated, pointeetype: not elaborated) are benign.

Yes, I've noticed that inconsistency too. But, seems like, this is an unrelated issue: TemplateSpecializationType type for some reasons uses id instead of qid (See also all other tests with templates in print-type.cpp, e.g. FieldDecl=baz). To get the type with nested name specifiers for the TemplateSpecializationType c-index-test should use the clang_getCanonicalType. But it doesn't use it for pointers. (Compare c-index-test.c:1289 and c-index-test.c:1335)

Here I'm also adding an another test to show that nested name specifiers are not lost.

Ok, so just for my understanding: the nested name specifier in the changed tests canonicaltype is *not* the user-written type (thus, what the elaborated type would get us), but the full nested name specifier of the canonical type?

AFAIUI the canonical type could never be an elaborated type, but just type could.

And yes before we used nested name specifier from the type name as written in the source code (a.k.a. ElaboratedType), now we use nested name specifier of the type that the elaborated type refers to.

skalinichev updated this revision to Diff 50027.Mar 8 2016, 1:31 AM
skalinichev updated this object.
skalinichev added reviewers: milianw, kfunk.

Ok, after some more testing I found out that elaborated types can have cv specifiers attached, but after using ET->getNamedType() we lose that information. So obviously the previous solution was wrong. Therefore I'm falling back to the original idea here once again: exposing the elaborated type. Also extending tests to account for the found bug.

kfunk added inline comments.Mar 10 2016, 10:01 AM
tools/libclang/CXType.cpp
990

I'm not happy with this name either. If you look through the libclang API, functions are named like clang_get${CPPCLASSNAME}${CPPMETHOD}. E.g. clang_getEnumConstantDeclValue, clang_getFieldDeclBitWidth, clang_getFunctionTypeCallingConv.

Sergey's initial version was closer to that. Maybe clang_getElaboratedTypeNamedType will just do...?

Looking a bit more at the recent commits in libclang, it seems like all recently added functions follow this naming convention:
clang_Cursor_*
clang_Type_*
e.t.c.

So I'm renaming the function to follow that style too.

Ok, If there are no objections, I'll commit this revision next week then.

This revision was automatically updated to reflect the committed changes.