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

[Loïc Jaquemet]

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.

* Implementation of sizeof, alignof and offsetof for libclang.
* Unit Tests
* Python bindings
* Python tests

-- 
Loïc Jaquemet
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130404/9cc8a646/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/20130404/9cc8a646/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/20130404/9cc8a646/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/20130404/9cc8a646/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/20130404/9cc8a646/attachment-0003.obj>