<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Loïc,</div><div><br></div><div>Thanks for your work! I have some comments:</div><div><br></div><div><blockquote type="cite"><div> /**</div><div>+ * \brief List the possible error codes for getTypeSizeOf and getTypeAlignOf.</div><div>+ *</div><div>+ * A value of this enumeration type can be returned if the target type is not</div><div>+ * \c a valid argument to sizeof or alignof.</div><div>+ */</div></blockquote></div><div><div><br></div></div><div>In the comments you should use the full function names with "\c" in front. You should also mention the getOffset-kind functions.</div><div><br></div><div><blockquote type="cite"><div>+ CXTypeLayoutError_InvalidFieldName = -5,</div><div>+ /**</div><div>+ * \brief One field is an incomplete Type.</div><div>+ */</div><div>+ CXTypeLayoutError_IncompleteField = -6,</div><div>+ /**</div><div>+ * \brief One field is a dependent Type.</div><div>+ */</div><div>+ CXTypeLayoutError_DependentField = -7,</div><div>+ /**</div><div>+ * \brief One field is not a constant size type.</div><div>+ */</div><div>+ CXTypeLayoutError_NotConstantSizeField = -8</div><div>+};</div></blockquote></div><div><div><br></div></div><div>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 ?</div><div>A record cannot be "NotConstantSize", this is for variable-length (non-constant) arrays, so CXTypeLayoutError_NotConstantSizeField can be removed.</div><div>And how about adding "Parent" to make it more clear about what they describe:</div><div><br></div><div>CXTypeLayoutError_IncompleteFieldParent</div><div>CXTypeLayoutError_DependentFieldParent</div><div><br></div><div><blockquote type="cite">+CINDEX_LINKAGE long long clang_getAlignOf(CXType T);</blockquote>...</div><div><blockquote type="cite">+CINDEX_LINKAGE long long clang_getSizeOf(CXType T);</blockquote></div><div>...</div><div><br></div><div>Please "prefix" the function names accordingly:</div><div><br></div><div>clang_getAlignOf -> clang_Type_getAlignOf</div><div>clang_isBitField -> clang_Cursor_isBitField</div><div><....></div><div><br></div><div><br></div><div><blockquote type="cite"><div>+/**</div><div>+ * \brief Return the offset of the field specified byt the cursor as it would be</div><div>+ * returned by __offsetof__</div></blockquote></div><div><div><br></div></div><div>This should mention that offset is returned in bits.</div><div><br></div><div><br></div><div><blockquote type="cite">+long long getOffsetOfFieldDecl(const FieldDecl * FD) {</blockquote></div><div><br></div><div>This should be static.</div><div><br></div><div><blockquote type="cite"><div>+long long clang_getOffsetOf(CXType PT, const char* S) {</div><div>+ // get the parent record type declaration</div><div>+ CXCursor PC = clang_getTypeDeclaration(PT);</div><div>+ if (clang_isInvalid(PC.kind))</div><div>+ return CXTypeLayoutError_Invalid;</div><div>+ const RecordDecl *RD =</div><div>+ dyn_cast_or_null<RecordDecl>(cxcursor::getCursorDecl(PC));</div><div>+ if (!RD)</div><div>+ return CXTypeLayoutError_Invalid;</div><div>+ RD = RD->getDefinition();</div><div>+ if (!RD)</div><div>+ return CXTypeLayoutError_Incomplete;</div></blockquote><div><br></div><div>Should this return CXTypeLayoutError_IncompleteFieldParent ?</div><div>Could you also check if the record is dependent before proceeding ?</div><br><blockquote type="cite"><div>+ // iterate the fields to get the matching name</div><div>+ StringRef fieldname = StringRef(clang_getCString(cxstring::createRef(S)));</div></blockquote><div><br></div><div>Just do:</div><div><br></div><div>StringRef fieldname = StringRef(S);</div><br><blockquote type="cite"><div>+ for (RecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();</div><div>+ I != E; ++I) {</div><div>+ if ( fieldname == (*I)->getName())</div><div>+ return getOffsetOfFieldDecl((*I));</div><div>+ }</div><div>+ return CXTypeLayoutError_InvalidFieldName; // FieldDecl is Null</div><div>+}</div></blockquote></div><div><div><br></div></div><div>Does this handle anonymous structs/unions ? For example:</div><div><br></div><div>struct Test {</div><div> struct {</div><div> union {</div><div> int foo;</div><div> };</div><div> };</div><div>};</div><div><br></div><div>If I pass "struct Test", "foo" will it work ? If yes, could you add a related test ?</div><div><br></div><div>-Argyrios</div><br><div><div>On Mar 15, 2013, at 12:08 AM, Loïc Jaquemet <<a href="mailto:loic.jaquemet@gmail.com">loic.jaquemet@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">Thanks for your time Dmitri. I'm sorry its taking so long for me to<br>get that right.<br><br><br><blockquote type="cite"><blockquote type="cite">Should sizeOf return -1 when the type is a bitfield ? [expr.sizeof] p1]<br></blockquote><br>Yes.<br></blockquote><br>Well, given the current signature of the function (CXType), we will<br>never be evaluating a field ( CXCursor). Only types.<br>So my guess, there is no specific case here after all.<br><br><blockquote type="cite"><blockquote type="cite">+ // Exceptions by GCC extension - see ASTContext.cpp:1313 getTypeInfoImpl<br>+ // if (QT->isFunctionType()) return 4;<br><br>I could not find the gcc extension reference document.<br></blockquote><br><a href="http://gcc.gnu.org/onlinedocs/gcc/C-Extensions.html">http://gcc.gnu.org/onlinedocs/gcc/C-Extensions.html</a><br><br>But apparently it does not cover this extension.<br></blockquote><br>Do I need to change the current situation for alignof? And to what ?<br>I expect the #15511 bug fix to fix it automatically.<br>Current output:<br>CXXMethod=foo:212:16 (Definition) (virtual) [type=void ()]<br>[typekind=FunctionProto] [sizeof=1] [alignof=4]<br><br><br><blockquote type="cite"><blockquote type="cite">+ // [expr.sizeof] p1: return -1 on: func, incomplete, bitfield,<br>incomplete enumeration<br>+ // [expr.sizeof] p3: pointer ok, function not ok.<br>+ // [gcc extension] lib/AST/ExprConstant.cpp:1372 HandleSizeof : vla == error<br>+ if (QT->isIncompleteType() || QT->isDependentType() ||<br>!QT->isConstantSizeType())<br>+ return -2;<br><br>I have to test for incompleteType and dependentType ( and constant<br>size type in sizeof) otherwise it asserts on templates.<br></blockquote><br>Could you use different error codes for these situations? A dumb<br>client can check return value >= 0, while a smart one can get more<br>information.<br></blockquote><br>Done.<br><br><blockquote type="cite"><br>-#define CINDEX_VERSION_MINOR 12<br>+#define CINDEX_VERSION_MINOR 13<br><br>Please refresh your patches to apply to trunk (we are at version 15 now :)<br><br></blockquote><br>Done<br><br><br><blockquote type="cite">/**<br>+ * \brief Return the alignment of a type in bytes as per<br>C++[expr.alignof] standard.<br><br>80 cols.<br><br></blockquote>[.. and bunch of others ]<br><br>done<br><br><blockquote type="cite">+ */<br>+CINDEX_LINKAGE long long clang_getRecordFieldOffsetInBits(CXCursor C);<br><br>It would help to add an enum with error codes.<br></blockquote><br>Done<br><br><blockquote type="cite"><br>But, there's other concern: what you need is to expose the record<br>layout. While a general sizeof() is useful, it will not work for you,<br>exactly because of all these exceptions. For example,<br>sizeof(reference type) is sizeof(type), but you need to know the<br>number of bytes the *reference itself* takes.<br></blockquote><br>Yes. I though about that, and for what I want to my achieve, I feel<br>that do not need an extra libclang function after all.<br>I will always query the type of the field, and have specific code for<br>reference and pointers anyway.<br>Other exceptions do not apply in the context.<br>So no need to clutter libclang with that.<br><br>I dropped the getRecordXXX functions in favor of sizeof/alignof/offsetof<br>I did put in two function for offsetof, because it can be queried both<br>with the Cursor and with the standard parent + fieldname combination.<br>And I do feel its an added value.<br><br><br><blockquote type="cite"><br>+long long clang_getRecordFieldOffsetInBits(CXCursor C) {<br>+ if (!clang_isDeclaration(C.kind))<br>+ return -1;<br>+ const FieldDecl *FD =<br>dyn_cast_or_null<FieldDecl>(cxcursor::getCursorDecl(C));<br>+ if (!FD)<br>+ return -1;<br>+ QualType QT = GetQualType(clang_getCursorType(C));<br>+ if (QT->isIncompleteType() || QT->isDependentType() ||<br>!QT->isConstantSizeType())<br><br>I don't see a test for isConstantSizeType case.<br></blockquote><br>Sadly I have no idea of an example for a non ConstantSizeType.<br>Any hint ?<br><br><blockquote type="cite"><br>+static enum CXChildVisitResult PrintTypeSize(CXCursor cursor, CXCursor p,<br>+ CXClientData d) {<br><br>Please align 'CXClientData' to 'CXCursor'.<br></blockquote><br>done<br><br><blockquote type="cite"><br>+static enum CXChildVisitResult PrintTypeSize(CXCursor cursor, CXCursor p,<br>+ CXClientData d) {<br>+ if (!clang_isInvalid(clang_getCursorKind(cursor))) {<br><br>Reverse the condition and de-nest:<br></blockquote><br>done<br><br>Added tests for bitfields too.<br><br><br><blockquote type="cite"><br>The tests could be reduced or reorganized to make them more targeted<br>(to check a specific thing). There's a lot of stuff that actually<br>checks MS struct layout algorithms.<br></blockquote><br>Cleaned up tests to be only composed of useful tests. Got rid of old<br>pasted tests.<br><br><blockquote type="cite"><br>Python bindings look good.<br></blockquote><br>added bitfields tests and offsetof<br><br><blockquote type="cite"><br>Dmitri<br><br>--<br>main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br></blockquote><br><br><br>--<span class="Apple-converted-space"> </span><br>Loïc Jaquemet<br><span><sizeof-alignof-offsetof-001></span><span><sizeof-alignof-offsetof-002-tests></span><span><sizeof-alignof-offsetof-003-python-bindings></span><span><sizeof-alignof-offsetof-004-python-bindings-tests></span>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div></blockquote></div><br></body></html>