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

[Argyrios Kyrtzidis]

Committed in r179251, thanks!

On Apr 9, 2013, at 4:08 PM, Loïc Jaquemet <loic.jaquemet at gmail.com> wrote:

> 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
> <sizeof-alignof-offsetof-001><sizeof-alignof-offsetof-002-tests><sizeof-alignof-offsetof-003-python-bindings><sizeof-alignof-offsetof-004-python-bindings-tests>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130410/365e0dbd/attachment.html>