Are these right ? test/Index/print-type-size.cpp still fails on top of r179099.
On Apr 5, 2013, at 2:20 PM, Loïc Jaquemet <[email protected]> wrote: > Outch. > Sorry for that. > > > 2013/4/5 Argyrios Kyrtzidis <[email protected]> > On Apr 4, 2013, at 6:33 PM, Loïc Jaquemet <[email protected]> wrote: > >> ah yes. >> The lookup failure. I forgot that one instance. >> >> Attached patches pass tests on r178827. > > 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) > >> >> >> 2013/4/4 Argyrios Kyrtzidis <[email protected]> >> >> 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> >> >> >> >> >> -- >> Loïc Jaquemet >> <expose-ast-record-layout-001><expose-ast-record-layout-002-tests><expose-ast-record-layout-003-python-bindings><expose-ast-record-layout-004-python-bindings-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
