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 <[email protected]> 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 <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
