[PATCH] [libclang] getSizeOf/getAlignOf/getOffsetOf (was [PATCH] Expose AST Record layout attributes to libclang)
Loïc Jaquemet
loic.jaquemet at gmail.com
Tue Apr 9 16:08:42 PDT 2013
Damn.
Ok, I double checked this time.
I must have manually transferred the old expose-* patches from my build
machine instead of the more recent sizeof* last time.
so this is the patches, as tested on top of r179132.
2013/4/9 Argyrios Kyrtzidis <akyrtzi at gmail.com>
> Are these right ? test/Index/print-type-size.cpp still fails on top of
> r179099.
>
> On Apr 5, 2013, at 2:20 PM, Loïc Jaquemet <loic.jaquemet at gmail.com> wrote:
>
> Outch.
> Sorry for that.
>
>
> 2013/4/5 Argyrios Kyrtzidis <akyrtzi at gmail.com>
>
>> On Apr 4, 2013, at 6:33 PM, Loïc Jaquemet <loic.jaquemet at gmail.com>
>> wrote:
>>
>> ah yes.
>> The lookup failure. I forgot that one instance.
>>
>> Attached patches pass tests on r178827.
>>
>>
>> I think you meant to attach different files, these look like earlier
>> versions of your patches (e.g. clang_getAlignOf, instead
>> of clang_Type_getAlignOf)
>>
>>
>>
>> 2013/4/4 Argyrios Kyrtzidis <akyrtzi at gmail.com>
>>
>>>
>>> On Apr 4, 2013, at 12:04 AM, Loïc Jaquemet <loic.jaquemet at gmail.com>
>>> wrote:
>>>
>>>
>>>
>>>
>>> 2013/4/1 Argyrios Kyrtzidis <akyrtzi at gmail.com>
>>>
>>>>
>>>> + /**
>>>> + * \brief One field in the record is an incomplete Type.
>>>> + */
>>>> + CXTypeLayoutError_IncompleteFieldParent = -6,
>>>> + /**
>>>> + * \brief One field in the record is a dependent Type.
>>>> + */
>>>> + CXTypeLayoutError_DependentFieldParent = -7
>>>> +};
>>>>
>>>> This was a bit confusing until I read
>>>>
>>>> + * If in the record there is another field's type declaration that is
>>>> + * an incomplete type, CXTypeLayoutError_IncompleteFieldParent is
>>>> returned.
>>>> + * If in the record there is another field's type declaration that is
>>>> + * a dependent type, CXTypeLayoutError_DependentFieldParent is
>>>> returned.
>>>> + */
>>>>
>>>> Could we change it to a simpler, "the parent record is
>>>> incomplete/dependent" ?
>>>>
>>>>
>>> Given the radical code change, these confusing errors do not exists any
>>> more.
>>>
>>>
>>>
>>>>
>>>> +/**
>>>> + * \brief Returns 1 if the cursor specifies a Record member that is a
>>>> bitfield.
>>>> + */
>>>> +CINDEX_LINKAGE unsigned clang_Cursor_isBitField(CXCursor C);
>>>>
>>>> the convention that we use is "Returns non-zero if ..."
>>>>
>>>>
>>> done
>>>
>>>
>>>
>>>>
>>>> +static long long visitRecordForNamedField(const RecordDecl *RD,
>>>> + StringRef FieldName) {
>>>> + for (RecordDecl::field_iterator I = RD->field_begin(), E =
>>>> RD->field_end();
>>>> + I != E; ++I) {
>>>>
>>> [..]
>>>
>>>> + return visitRecordForNamedField(RD, FieldName);
>>>> +}
>>>>
>>>> I think there is a simpler and more efficient way to handle fields in
>>>> anonymous records, something like this:
>>>> Inside clang_Type_getOffsetOf():
>>>>
>>>> CXTranslationUnit TU =
>>>> static_cast<CXTranslationUnit>(const_cast<void*>(PT.data[1]));
>>>> ASTContext &Ctx = cxtu::getASTUnit(TU)->getASTContext();
>>>> IdentifierInfo *II = &Ctx.Idents.get(S);
>>>> DeclarationName FieldName(II);
>>>> RecordDecl::lookup_const_result Res = RD->lookup(FieldName);
>>>> if (Res.size() != 1)
>>>> return CXTypeLayoutError_InvalidFieldName;
>>>> if (const FieldDecl *FD = dyn_cast<FieldDecl>(Res.front()))
>>>> return getOffsetOfFieldDecl(FD);
>>>> if (const IndirectFieldDecl *IFD =
>>>> dyn_cast<IndirectFieldDecl>(Res.front()))
>>>> return Ctx.getFieldOffset(IFD); // Change getOffsetOfFieldDecl()
>>>> to accept IFD.
>>>>
>>>> return CXTypeLayoutError_InvalidFieldName;
>>>>
>>>>
>>> Thanks! That was exactly was I was looking for.
>>>
>>>
>>> In the process of implementing that new code, I stumble on some new
>>> crash tests cases.
>>> The RecordLayoutBuilder forces me to do a full validation of all records
>>> fields in a record.
>>> I have implemented a recursive validation function to do that.
>>> At the end, it does simplify the testing quite a lot.
>>> I do have to forget about the two previously confusing error types, as
>>> they would not be distinguishable.
>>>
>>> So, basically, this code is now simpler and more robust.
>>> I added some tests cases in the Incomplete namespace to demonstrate the
>>> several issues I uncovered.
>>>
>>>
>>>
>>>>
>>>>
>>>> I also removed the duplicate clang_Cursor_getOffsetOf().
>>>> After consideration, it did not make sense, especially in the
>>>> anonymous record situation.
>>>>
>>>>
>>>> Not sure about this, clang_Cursor_getOffsetOf is arguable more useful
>>>> than clang_Type_getOffsetOf.
>>>> Let's say you have this use-case: "visit all fields in a record and get
>>>> their offsets". To do this (as your changes in c-index-test show) you need
>>>> to use this roundabout way where, you have the field, then you get its
>>>> name, and pass it to clang_Type_getOffsetOf which looks for the same field.
>>>> Can't clang_Cursor_getOffsetOf just work, for example if you have a
>>>> cursor for "foo" in
>>>>
>>>> struct S {
>>>> struct {
>>>> int foo;
>>>> };
>>>> };
>>>>
>>>> it should just return the offset of "foo" inside "struct S".
>>>>
>>>
>>> That was also my feeling at the beginning.
>>> But after several iteration on my own code, I see that my own use of
>>> this function is always in a context were I do have the record's type and
>>> the field's name at hand.
>>> On top of that, the C++ standard calls for a Type signature.
>>> So I will keep it to that.
>>>
>>>
>>> Please see attached diffs.
>>>
>>>
>>> test/Index/print-type-size.cpp failed when I applied the diffs on top of
>>> r178800, could you take a look ?
>>>
>>>
>>> * Implementation of sizeof, alignof and offsetof for libclang.
>>> * Unit Tests
>>> * Python bindings
>>> * Python tests
>>>
>>> --
>>> Loïc Jaquemet
>>> <sizeof-alignof-offsetof-001><sizeof-alignof-offsetof-002-tests>
>>> <sizeof-alignof-offsetof-003-python-bindings>
>>> <sizeof-alignof-offsetof-004-python-bindings-tests>
>>>
>>>
>>>
>>
>>
>> --
>> Loïc Jaquemet
>> <expose-ast-record-layout-001><expose-ast-record-layout-002-tests>
>> <expose-ast-record-layout-003-python-bindings>
>> <expose-ast-record-layout-004-python-bindings-tests>
>>
>>
>>
>
>
> --
> Loïc Jaquemet
> <sizeof-alignof-offsetof-001><sizeof-alignof-offsetof-002-tests>
> <sizeof-alignof-offsetof-003-python-bindings>
> <sizeof-alignof-offsetof-004-python-bindings-tests>
>
>
>
--
Loïc Jaquemet
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/74e9dfb3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-001
Type: application/octet-stream
Size: 9162 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/74e9dfb3/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-002-tests
Type: application/octet-stream
Size: 19689 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/74e9dfb3/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-003-python-bindings
Type: application/octet-stream
Size: 1950 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/74e9dfb3/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-004-python-bindings-tests
Type: application/octet-stream
Size: 3468 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/74e9dfb3/attachment-0003.obj>
More information about the cfe-commits
mailing list