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

[Hal Finkel]

Dmitri,

Thanks again! I think that this is going to be a great feature.

I would still like to remove the remaining MPI-specific things. Also, I
think that the category tag should appear in the warning messages.

Specifically, we currently have:
> +def warn_mpi_type_mismatch : Warning<
> +  "actual buffer element type %0 doesn't match specified
> MPI_Datatype "
> +  "(which is %1)">, InGroup<TypeSafety>;
and also the generic message:
> +def warn_type_safety_pointee_type_mismatch : Warning<
> +  "pointee type %0 doesn't match specified type tag %1">,
> InGroup<TypeSafety>;
I think that in both cases we should use the generic message, but the
generic message should contain the category name:
+def warn_type_safety_pointee_type_mismatch : Warning<
+  "pointee type %0 doesn't match the specified %1 type tag %2">,
InGroup<TypeSafety>;
so it will print as 
pointee type foo dosen't match the specified mpi type tag
MPI_DATATYPE_BAR.

Again, for this message:
+def warn_mpi_datatype_null_but_buffer_not_null : Warning<
+  "MPI_DATATYPE_NULL was specified but buffer is not a null pointer">,
+  InGroup<TypeSafety>;
there is no reason for this to be a separate message just for MPI. The
message should print the offending tag so that things are clear to the
user.

And finally, because I'd like it to be as easy as possible for library
maintainers, please add an example in the documentation showing how to
properly guard the attributes (ifdef + has_attribute).

To the rest of the list: Can one of the clang code owners please look
at this?

 -Hal

On Sun, 27 May 2012 20:48:19 +0300
Dmitri Gribenko <gribozavr at gmail.com> wrote:

> Hi Hal,
> 
> Thank you for the review.  Comments inline.
> 
> Updated version of the patch attached.  Changes include addressing the
> review comments and proper handling of 128-bit integer constants in
> attribute arguments and type tags (ensures we don't crash in these
> cases).
> 
> On Thu, May 24, 2012 at 5:27 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> > layout_compatible - The documentation on this needs to be expanded.
> > I recommend an example or two (at least).
> 
> Added an example.  Is that enough or more explanations are needed?
> 
> > Do we need a new has_feature check so that we can properly guard the
> > new attributes in public headers?
> 
> __has_attribute already works with these attributes.
> 
> > Are C++ objects that are implicitly convertible to pointers of the
> > correct type handled correctly [this is not obvious to me from
> > reading the patch, but I'm not an expert here]?
> 
> Classes with conversion operators, like this:
> class OperatorIntStar
> {
> public:
>   operator int*();
> };
> These classes are supported.  I've added a test for that to make sure
> we don't regress.
> 
> >> +  bool IsMPI = ArgumentKind->isStr("mpi");
> >> +
> >> +  if (SpecifiedType->isVoidType()) {
> >> +    // Type tag with matching void type requires a null pointer.
> >> +    if (!ArgumentExpr->isNullPointerConstant(Context,
> >> +
> >> Expr::NPC_ValueDependentIsNotNull)) {
> >> +      Diag(ArgumentExpr->getExprLoc(),
> >> +           IsMPI ?
> >> diag::warn_mpi_datatype_null_but_buffer_not_null
> >> +                 : diag::warn_type_safety_null_pointer_required)
> >> +          << ArgumentExpr->getSourceRange()
> >> +          << TypeTagExpr->getSourceRange();
> >> +    }
> >> +    return;
> >> +  }
> >
> > Can we generalize this as well? Maybe we should call this
> > void_must_be_null, and make it like layout_compatible (an optional
> > parameter)? [As far as I can tell, this is the only MPI-specific
> > logic left, is there anything else?]
> 
> Done.
> Yes, that was the only piece of MPI-specific logic.
> 
> Dmitri
> 



-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory