[cfe-commits] [PATCH] Type safety attributes (was: Compile-time MPI_Datatype checking)

[Douglas Gregor]

On Aug 4, 2012, at 10:52 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Sat, Aug 4, 2012 at 10:42 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> On Tue, Jun 12, 2012 at 10:46 AM, Jordan Rose <jordan_rose at apple.com> 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