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.
+ case Stmt::BinaryConditionalOperatorClass:
+ case Stmt::ConditionalOperatorClass: {
+ const AbstractConditionalOperator *ACO =
+ cast<AbstractConditionalOperator>(TypeExpr);
+ bool Result;
+ if (ACO->getCond()->EvaluateAsBooleanCondition(Result, Ctx)) {
+ if (Result)
+ TypeExpr = ACO->getTrueExpr();
+ else
+ TypeExpr = ACO->getFalseExpr();
+ continue;
+ }
+ }
Unintended fallthrough here? If we can't evaluate the condition at
compile-time, we probably failed (return false).
+ case Stmt::BinaryOperatorClass: {
+ const BinaryOperator *BO = cast<BinaryOperator>(TypeExpr);
+ if (BO->getOpcode() == BO_Comma) {
+ TypeExpr = BO->getRHS();
+ continue;
+ }
+ }
Ditto, although this time the fallthrough is to a "return false" so it would
work as intended.
+ if (const ArgumentWithTypeTagAttr *A =
+ dyn_cast<ArgumentWithTypeTagAttr>(Attr)) {
+ ArgumentKind = A->getArgumentKind();
+ TypeTagIdx = A->getTypeTagIdx();
+ ArgumentIdx = A->getArgumentIdx();
+ IsPointerAttr = false;
+ } else if (const PointerWithTypeTagAttr *A =
+ dyn_cast<PointerWithTypeTagAttr>(Attr)) {
+ ArgumentKind = A->getPointerKind();
+ TypeTagIdx = A->getTypeTagIdx();
+ ArgumentIdx = A->getPointerIdx();
+ IsPointerAttr = true;
There's a few places with code like this. Why not just use one attribute kind
for both ArgumentWithTypeTagAttr and PointerWithTypeTagAttr, and have the
attribute have an ExpectsPointer flag?
+ if (!ArgumentExpr->isNullPointerConstant(Context,
+
Expr::NPC_ValueDependentIsNotNull)) {
If the type tag being passed is also value-dependent, this might be a spurious
warning (although that's a kind of scary way to code). I think you'd also get
the warning again when the template is instantiated, so I would consider using
NPC_ValueDependentIsNull. You should probably have a test case for this, though.
+ // C++11 [basic.fundamental] p1:
+ // Plain char, signed char, and unsigned char are three distinct types.
+ //
+ // But we treat plain `char' as equivalent to `signed char' or `unsigned
+ // char' depending on the current char signedness mode.
+ if(mismatch &&
+ isa<BuiltinType>(ArgumentType) && isa<BuiltinType>(RequiredType)) {
+ BuiltinType::Kind PointeeKind =
cast<BuiltinType>(ArgumentType)->getKind();
+ BuiltinType::Kind RequiredKind =
cast<BuiltinType>(RequiredType)->getKind();
+ if((PointeeKind == BuiltinType::SChar && RequiredKind ==
BuiltinType::Char_S) ||
+ (PointeeKind == BuiltinType::UChar && RequiredKind ==
BuiltinType::Char_U) ||
+ (PointeeKind == BuiltinType::Char_U && RequiredKind ==
BuiltinType::UChar) ||
+ (PointeeKind == BuiltinType::Char_S && RequiredKind ==
BuiltinType::SChar))
+ mismatch = false;
You don't do this for isLayoutCompatible, though. Are these two structs
layout-compatible under -fsigned-char?
struct A { char x; };
struct B { signed char x; };
To me, using isa<> and then cast<> is less pretty than using dyn_cast, even
though you'd have to do both dyn_casts before the condition.
Do we want to treat a required type of 'char' as equivalent to /any/ char type?
Usually someone using a type_tag for 'char' would be passing either raw bytes
or ASCII (or UTF-8) characters, neither of which really have inherent
signedness. OTOH, if the signedness is in the required type than I think being
strict is correct.
Finally, that giant condition can probably go in a helper function, especially
if we're doing the same thing somewhere else in Sema.
+ if (!Attr.getParameterName()) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_not_identifier)
+ << Attr.getName() << 1;
+ return;
+ }
+
+ if (Attr.getNumArgs() != 2) {
+ S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments) << 3;
+ return;
+ }
Please describe in an inline comment what the magic numbers 1 and 3 indicate
for these diagnostics.
+ if (Pointer)
+ D->addAttr(::new (S.Context) PointerWithTypeTagAttr(Attr.getRange(),
+ S.Context,
+ PointerKind,
+ PointerIdx,
+ TypeTagIdx));
+ else
+ D->addAttr(::new (S.Context) ArgumentWithTypeTagAttr(Attr.getRange(),
+ S.Context,
+ PointerKind,
+ PointerIdx,
+ TypeTagIdx));
Again, I think it's a good idea to unify these.
+ struct TypeTagForDatatypeData {
+ ParsedType *MatchingCType;
+ unsigned LayoutCompatible : 1;
+ unsigned MustBeNull : 1;
+ };
This is a little silly, but can we cram these into a PointerIntPair? Right now
we're spending two words where we could get away with 1. (Or, even if we pass
the data around in struct format, we could be /storing/ it in a single word.)
And finally, some warning wording suggestions:
+def err_pointer_with_type_tag_not_pointer : Error<
+ "pointer argument is not of a pointer type">;
I would actually use warn_nonnull_pointers_only, and change it to take the name
of the attribute, so that you can say "'pointer_with_type_tag attribute only
applies to pointer arguments".
+def warn_type_tag_for_datatype_not_ice : Warning<
+ "'type_tag_for_datatype' attribute requires the initializer to be "
+ "an %select{integer|integral}0 constant expression; "
+ "initializer ignored by attribute">, InGroup<TypeSafety>;
This one's mostly okay, but "initializer ignored by attribute" is making me
wonder if it really means "initializer /discarded/ by attribute", which would
affect correctness. Perhaps the last clause should just be removed?
+def warn_type_tag_for_datatype_wrong_kind : Warning<
+ "this type tag was not designed to be used with this function">,
+ InGroup<TypeSafety>;
Let's be helpful here and say something like "type tag 'D_tag' is associated
with SYSTEM 'd', but function/method 'C_func' uses SYSTEM 'c'". Clearly this
isn't perfect; I'm not sure what to say instead of SYSTEM. "Type schema"? "Type
family"?
+def warn_type_safety_type_mismatch : Warning<
+ "argument type %0 doesn't match specified %1 type tag "
+ "%select{that requires %3|}2">, InGroup<TypeSafety>;
+def warn_type_safety_pointee_type_mismatch : Warning<
+ "pointee type %0 doesn't match specified %1 type tag "
+ "%select{that requires %3|}2">, InGroup<TypeSafety>;
+def warn_type_safety_null_pointer_required : Warning<
+ "specified %0 type tag requires a null pointer">, InGroup<TypeSafety>;
+
I like the last one here, though maybe it should have single quotes around the
type family name. The other two could be more descriptive, though:
"argument is required to %select{have|be layout-compatible with}2 type '%1' by
type tag '%0'"
"argument is required to be a pointer to type '%1' by type tag '%0'"
"argument is required to be null by type tag '%0'"
I think having the name of the type tag in the warning makes things clearer,
especially if you accidentally put the wrong type tag. (Since most type tags
have a prefix, the type family would be redundant) And none of our other
diagnostics use the word "pointee", which sounds funny to me.
My weird argument numbering is for consistency among the three warnings, but
that might be more confusing than it's worth.
+def err_type_safety_unknown_flag : Error<
+ "%0 is not a type comparison flag; use 'layout_compatible' or
'must_be_null'">;
+
Bike-shedding suggestion: "invalid type comparison flag '%0'". I'm not sure if
the suggestions for valid flags is necessary, but this is a new annotation that
people will not be familiar with.
(If you want to get fancy here, add typo detection.)
--
I think this is a really useful addition to a compiler. Off the top of my head
I can think of one more major potential user of these annotations: OpenGL. I'd
really like to see this go in.
I have a couple comments on your docs but I think this is quite enough for now.
Jordan
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
