On Apr 4, 2013, at 12:04 AM, Loïc Jaquemet <[email protected]> wrote:
> > > > 2013/4/1 Argyrios Kyrtzidis <[email protected]> > > + /** > + * \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>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
