On Aug 4, 2012, at 10:52 PM, Dmitri Gribenko <[email protected]> wrote:
> On Sat, Aug 4, 2012 at 10:42 PM, Dmitri Gribenko <[email protected]> wrote: >> On Tue, Jun 12, 2012 at 10:46 AM, Jordan Rose <[email protected]> wrote: >>> Hi, Dmitri. I'd like for this patch not to get lost with your new >>> comment-parsing work, though it might be too late for that. >> >> Thank you for the review! Finally, I got back to this. > > [...] > > Hi Doug, > > Could you comment on this, please? If I understand correctly, I'll > need your review to commit this since this adds new attributes. > > In support of this patch, I should say that MPICH2 developers have > already committed the corresponding patch for mpi.h and they build > their own version of clang with this patch to check their MPI > projects. This is a good extension. Even if the MPI community isn't large enough to justify the addition, the API design pattern that this set of attributes describes is fairly common in the C world. The patch looks good, and I have just a few minor comments: + llvm::APSInt MagicValueInt; + if (!MagicValueExpr->isIntegerConstantExpr(MagicValueInt, Context)) { + Diag(I->getRange().getBegin(), + diag::warn_type_tag_for_datatype_not_ice) + << LangOpts.CPlusPlus << MagicValueExpr->getSourceRange(); + continue; + } + uint64_t MagicValue = MagicValueInt.getZExtValue(); Should check for > 64 active bits in the magic value. +bool isLayoutCompatibleStruct(ASTContext &C, + RecordDecl *RD1, + RecordDecl *RD2) { + // If both records are C++ classes, check that base classes match. + if (const CXXRecordDecl *D1CXX = dyn_cast<CXXRecordDecl>(RD1)) { + if (const CXXRecordDecl *D2CXX = dyn_cast<CXXRecordDecl>(RD2)) { CXXRecordDecls are used in C++, bare RecordDecls are used in C, so if one of the RecordDecls is a CXXRecordDecl, you know that the other one is, so you can just cast<CXXRecordDecl>(RD2) an eliminate the inner "if". + /// \brief A map from magic value to associated dataype. We can not use + /// DenseMap here because it reserves two key values for its internal needs. + typedef std::map<uint64_t, TypeTagData> TypeTagForDatatypeMagicValuesType; + +private: + /// \brief A map from ArgumentKind identifier name to registered magic + /// values. + OwningPtr<llvm::StringMap<TypeTagForDatatypeMagicValuesType> > + TypeTagForDatatypeMagicValues; This is a really heavy data structure. Could we perhaps use a single DenseMap<std::pair<IdentifierInfo*, uint64_t>, TypeTagData>, which has much smaller value types? + if (VD) { + for (specific_attr_iterator<TypeTagForDatatypeAttr> + I = VD->specific_attr_begin<TypeTagForDatatypeAttr>(), + E = VD->specific_attr_end<TypeTagForDatatypeAttr>(); + I != E; ++I) { + if (I->getPointerKind() != PointerKind) { + FoundWrongKind = true; + return false; + } + TypeInfo.Type = I->getMatchingCType(); + TypeInfo.LayoutCompatible = I->getLayoutCompatible(); + TypeInfo.MustBeNull = I->getMustBeNull(); + return true; + } + return false; + } else { Since you're returning along every path in the if (VD) case, I suggest de-nesting the rest. +namespace { +bool IsSameCharType(QualType T1, QualType T2) { + if (!isa<BuiltinType>(T1) || + !isa<BuiltinType>(T2)) + return false; + + BuiltinType::Kind T1Kind = cast<BuiltinType>(T1)->getKind(); + BuiltinType::Kind T2Kind = cast<BuiltinType>(T2)->getKind(); Do you mean to use T1->getAs<BuiltinType>() and T2->getAs<BuiltinType>() here? It's not clear that IsSameCharType is always getting canonical types. Everything else looks great, thanks! - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
