[PATCH] [libclang] getSizeOf/getAlignOf/getOffsetOf (was [PATCH] Expose AST Record layout attributes to libclang)

Fri Apr 5 14:20:17 PDT 2013

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130405/7ff32e88/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/20130405/7ff32e88/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/20130405/7ff32e88/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/20130405/7ff32e88/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/20130405/7ff32e88/attachment-0003.obj>