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

[Loïc Jaquemet]

2013/3/18 Argyrios Kyrtzidis <akyrtzi at gmail.com>:
> Hi Loïc,
>
> Thanks for your work! I have some comments:

Thanks

>
>  /**
> + * \brief List the possible error codes for getTypeSizeOf and
> getTypeAlignOf.
> + *
> + * A value of this enumeration type can be returned if the target type is
> not
> + * \c a valid argument to sizeof or alignof.
> + */
> In the comments you should use the full function names with "\c" in front.
> You should also mention the getOffset-kind functions.

Done

>
> +  CXTypeLayoutError_InvalidFieldName = -5,
> +  /**
> +   * \brief One field is an incomplete Type.
> +   */
> +  CXTypeLayoutError_IncompleteField = -6,
> +  /**
> +   * \brief One field is a dependent Type.
> +   */
> +  CXTypeLayoutError_DependentField = -7,
> +  /**
> +   * \brief One field is not a constant size type.
> +   */
> +  CXTypeLayoutError_NotConstantSizeField = -8
> +};
>
>
> This confused me about what is the difference between
> CXTypeLayoutError_Incomplete and CXTypeLayoutError_IncompleteField. Via
> reading the code I assume these refer to the parent record that the field
> belongs to, right ?
> A record cannot be "NotConstantSize", this is for variable-length
> (non-constant) arrays, so CXTypeLayoutError_NotConstantSizeField can be
> removed.
> And how about adding "Parent" to make it more clear about what they
> describe:
>
> CXTypeLayoutError_IncompleteFieldParent
> CXTypeLayoutError_DependentFieldParent

I agree.
code and Test updated, I now added checks for errors codes returns.
enum updated.

I'm not sure about the result though:
struct CS1 {
   int f1[i];
   float f2;
 };

Gives:
StructDecl=CS1:262:11 (Definition) [type=CS1] [typekind=Record]
[sizeof=1] [alignof=1]
FieldDecl=f1:265:9 (Definition) [type=int [i]] [typekind=Unexposed]
[sizeof=-4] [alignof=4] [offsetof=0]
DeclRefExpr=i:258:12 [type=int] [typekind=Int] [sizeof=4] [alignof=4]
FieldDecl=f2:267:11 (Definition) [type=float] [typekind=Float]
[sizeof=4] [alignof=4] [offsetof=0]

should offsetof return 0 for f1 or an error ?
should offsetof return 0 for f2 or an error ?
Is that even a valid non-constant-size test ?
should offsetof return anything with an invalid context ?


> +CINDEX_LINKAGE long long clang_getAlignOf(CXType T);
> +CINDEX_LINKAGE long long clang_getSizeOf(CXType T);
> Please "prefix" the function names accordingly:
>
> clang_getAlignOf -> clang_Type_getAlignOf
> clang_isBitField -> clang_Cursor_isBitField
> <....>

Done

>
> +/**
> + * \brief Return the offset of the field specified byt the cursor as it
> would be
> + *   returned by __offsetof__
> This should mention that offset is returned in bits.

done
> +long long getOffsetOfFieldDecl(const FieldDecl * FD) {
>
>
> This should be static.

done

>
> +long long clang_getOffsetOf(CXType PT, const char* S) {
> +  // get the parent record type declaration
[..]
> +  if (!RD)
> +    return CXTypeLayoutError_Incomplete;
>
>
> Should this return CXTypeLayoutError_IncompleteFieldParent ?
yes
> Could you also check if the record is dependent before proceeding ?
Hum. yeah. It does duplicate these tests tough. But I will go with "fail early".

> +  // iterate the fields to get the matching name
> +  StringRef fieldname = StringRef(clang_getCString(cxstring::createRef(S)));
>
> Just do:
>
> StringRef fieldname = StringRef(S);
ok

>
> +  for (RecordDecl::field_iterator I = RD->field_begin(), E =
> RD->field_end();
> +       I != E; ++I) {
> +    if ( fieldname == (*I)->getName())
> +      return getOffsetOfFieldDecl((*I));
> +  }
> +  return CXTypeLayoutError_InvalidFieldName; // FieldDecl is Null
> +}
>
>
> Does this handle anonymous structs/unions ? For example:
>
> struct Test {
>   struct {
>     union {
>       int foo;
>     };
>   };
> };
>
> If I pass "struct Test", "foo" will it work ? If yes, could you add a
> related test ?
>

It does not work.
I will look into that.


> -Argyrios
>

-- 
Loïc Jaquemet
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-001
Type: application/octet-stream
Size: 10694 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130319/cb8970e1/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-002-tests
Type: application/octet-stream
Size: 16122 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130319/cb8970e1/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-003-python-bindings
Type: application/octet-stream
Size: 2213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130319/cb8970e1/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeof-alignof-offsetof-004-python-bindings-tests
Type: application/octet-stream
Size: 3231 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130319/cb8970e1/attachment-0003.obj>