<div dir="ltr"><div><div>Damn.<br></div>Ok, I double checked this time.<br>I must have manually transferred the old expose-* patches from my build machine instead of the more recent sizeof* last time.<br><br></div>so this is the patches, as tested on top of r179132.<br>
<div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/4/9 Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Are these right ? test/Index/print-type-size.cpp still fails on top of r179099.</div><br><div><div><div class="h5"><div>On Apr 5, 2013, at 2:20 PM, Loïc Jaquemet <<a href="mailto:loic.jaquemet@gmail.com" target="_blank">loic.jaquemet@gmail.com</a>> wrote:</div>
<br></div></div><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><div class="h5"><div dir="ltr">Outch.<br>Sorry for that.<br>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/4/5 Argyrios Kyrtzidis<span> </span><span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>On Apr 4, 2013, at 6:33 PM, Loïc Jaquemet <<a href="mailto:loic.jaquemet@gmail.com" target="_blank">loic.jaquemet@gmail.com</a>> wrote:<br></div><div><div><br><blockquote type="cite">
<div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div dir="ltr"><div><div>ah yes.<br></div>The lookup failure. I forgot that one instance.<br><br>
</div>Attached patches pass tests on r178827.<br></div></div></blockquote><div><br></div></div><div>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)</div>
<br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/4/4 Argyrios Kyrtzidis<span> </span><span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Apr 4, 2013, at 12:04 AM, Loïc Jaquemet <<a href="mailto:loic.jaquemet@gmail.com" target="_blank">loic.jaquemet@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div class="gmail_extra"><br><br><br><div class="gmail_quote">2013/4/1 Argyrios Kyrtzidis<span> </span><span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div>+ /**</div><div>+ * \brief One field in the record is an incomplete Type.</div><div>+ */</div><div>+ CXTypeLayoutError_IncompleteFieldParent = -6,</div><div>+ /**</div>
<div>+ * \brief One field in the record is a dependent Type.</div><div>+ */</div><div>+ CXTypeLayoutError_DependentFieldParent = -7</div><div>+};</div><div><br></div><div>This was a bit confusing until I read</div><div>
<br></div><div><div>+ * If in the record there is another field's type declaration that is</div><div>+ * an incomplete type, CXTypeLayoutError_IncompleteFieldParent is returned.</div><div>+ * If in the record there is another field's type declaration that is</div>
<div>+ * a dependent type, CXTypeLayoutError_DependentFieldParent is returned.</div><div>+ */</div></div><div><br></div><div>Could we change it to a simpler, "the parent record is incomplete/dependent" ?</div>
<div><br></div></div></div></blockquote><div><br></div><div>Given the radical code change, these confusing errors do not exists any more.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div></div><div><br></div><div><div>+/**</div><div>+ * \brief Returns 1 if the cursor specifies a Record member that is a bitfield.</div><div>+ */</div><div>+CINDEX_LINKAGE unsigned clang_Cursor_isBitField(CXCursor C);</div>
</div><div><br></div><div>the convention that we use is "Returns non-zero if ..."</div><div><br></div></div></blockquote><div><br>done<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div></div><div><br></div><div><div>+static long long visitRecordForNamedField(const RecordDecl *RD,</div><div>+ StringRef FieldName) {</div><div>
<div>+ for (RecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();</div><div>+ I != E; ++I) {</div></div></div></div></blockquote><div>[..]<span> </span><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div>+ return visitRecordForNamedField(RD, FieldName);<div>+}</div></div><div><br></div><div>I think there is a simpler and more efficient way to handle fields in anonymous records, something like this:</div>
<div>Inside clang_Type_getOffsetOf():</div><div><br></div><div> <span> </span>CXTranslationUnit TU =<br> <span> </span>static_cast<CXTranslationUnit>(const_cast<void*>(PT.data[1]));<br> <span> </span>ASTContext &Ctx = cxtu::getASTUnit(TU)->getASTContext();<br>
<span> </span>IdentifierInfo *II = &Ctx.Idents.get(S);<br> <span> </span>DeclarationName FieldName(II);<br> <span> </span>RecordDecl::lookup_const_result Res = RD->lookup(FieldName);<br> <span> </span>if (Res.size() != 1)<br>
<span> </span>return CXTypeLayoutError_InvalidFieldName;<br> <span> </span>if (const FieldDecl *FD = dyn_cast<FieldDecl>(Res.front()))<br> <span> </span>return getOffsetOfFieldDecl(FD);<br> <span> </span>if (const IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(Res.front()))<br>
<span> </span>return Ctx.getFieldOffset(IFD); // Change getOffsetOfFieldDecl() to accept IFD.<br><br> <span> </span>return CXTypeLayoutError_InvalidFieldName;<br><br></div></div></blockquote><div><br></div><div>Thanks! That was exactly was I was looking for.<br>
</div><div><br><br>In the process of implementing that new code, I stumble on some new crash tests cases.<br></div><div>The RecordLayoutBuilder forces me to do a full validation of all records fields in a record.<br></div>
<div>I have implemented a recursive validation function to do that.<br></div><div>At the end, it does simplify the testing quite a lot.<br></div><div>I do have to forget about the two previously confusing error types, as they would not be distinguishable.<br>
</div><div><br></div><div>So, basically, this code is now simpler and more robust.<br></div><div>I added some tests cases in the Incomplete namespace to demonstrate the several issues I uncovered.<br></div><div><br> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div></div><div><br>
</div></div><div><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br>I also removed the duplicate clang_Cursor_getOffsetOf().<br>
After consideration, it did not make sense, especially in the<br>anonymous record situation.<br></div></blockquote><div><br></div></div><div>Not sure about this, clang_Cursor_getOffsetOf is arguable more useful than clang_Type_getOffsetOf.</div>
<div>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.</div>
<div>Can't clang_Cursor_getOffsetOf just work, for example if you have a cursor for "foo" in</div><div><br></div><div>struct S {</div><div> struct {</div><div> <span> </span>int foo;</div><div> };</div>
<div>};</div><div><br></div><div>it should just return the offset of "foo" inside "struct S".</div></div></blockquote><div><br></div><div>That was also my feeling at the beginning.<br></div><div>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.<br>
</div><div>On top of that, the C++ standard calls for a Type signature.<br></div><div>So I will keep it to that.<span> </span><br></div><br></div><br>Please see attached diffs.<br></div></div></blockquote><div><br></div></div>
<div>test/Index/print-type-size.cpp failed when I applied the diffs on top of r178800, could you take a look ?</div><br><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div class="gmail_extra"><br>* Implementation of sizeof, alignof and offsetof for libclang.<br>* Unit Tests<br>* Python bindings<br>* Python tests<br><br>--<span> </span><br>Loïc Jaquemet<br></div></div></div><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></blockquote>
</div><br></div></blockquote></div><br><br clear="all"><br>--<span> </span><br>Loïc Jaquemet<br></div></div></div><span><expose-ast-record-layout-001></span><span><expose-ast-record-layout-002-tests></span><span><expose-ast-record-layout-003-python-bindings></span><span><expose-ast-record-layout-004-python-bindings-tests></span></div>
</blockquote></div><br></div></blockquote></div><br><br clear="all"><br>--<span> </span><br>Loïc Jaquemet<br></div></div></div><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></div>
</blockquote></div><br></div></blockquote></div><br><br clear="all"><br>-- <br>Loïc Jaquemet<br>
</div>