This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH 3/5] Atomic type qualifier - front end changes
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Andrew MacLeod <amacleod at redhat dot com>
- Cc: <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 9 Aug 2013 00:05:55 +0000
- Subject: Re: [PATCH 3/5] Atomic type qualifier - front end changes
- References: <51F2AEB1 dot 60408 at redhat dot com> <51F7EE86 dot 20805 at redhat dot com> <51F7F448 dot 2010901 at redhat dot com>
Observations on this patch:
* build_qualified_type sets the qualifiers to exactly the set specified.
Thus, it looks like your handle_atomic_attribute will remove existing
qualifiers when adding "atomic".
* c-aux-info.c is meant to be generating actual valid C declarations, I
think, meaning _Atomic rather than "atomic".
* The code you have checking for _Atomic with function declarators appears
to be in the wrong place. What you're testing is the combination of
(_Atomic appears in declaration specifiers, as given by atomicp) and (some
declarator in the nested sequence of declarators is a function
declarator). But there are valid cases for this - for example a function
returning a pointer to atomic. And there are invalid cases involving an
atomic-qualified function type that you don't catch there - for example,
if _Atomic is applied to a typedef for a function type rather than
together with the declarator.
The relevant issue is whether the *function type* itself gets qualified.
There are already pedwarns-if-pedantic for this, given that there's
undefined behavior in ISO C for qualifiers on function types in general,
but making this case a hard error seems reasonable enough. To do that,
you need to adjust the cases
/* Merge any constancy or volatility into the target type
for the pointer. */
if (pedantic && TREE_CODE (type) == FUNCTION_TYPE
pedwarn (loc, OPT_Wpedantic,
"ISO C forbids qualified function types");
and similar for typedefs, type names, parameters and actual function
declarations, to give such errors. And similarly, for all the various
cases of things that can be declared, whenever qualifiers get applied in
grokdeclarator you need to ensure an error is given if the type is an
array type, since that is also a constraint violation in ISO C.
* The pedwarn for _Atomic outside C11 mode should I think only be a
pedwarn-if-pedantic, so pass OPT_Wpedantic instead of 0. And the text
should match other such pedwarns (i.e. "ISO C90 does not support _Atomic"
or "ISO C99 does not support _Atomic", depending on the standard version
* The parser comments with syntax should refer to _Atomic not "atomic".
* You change convert_for_assignment regarding discarding qualifiers.
Doing so is correct as far as it goes - not because of the reason in your
comment, but because in C11 terms _Atomic isn't a type qualifier. But I
don't think the change is enough. Because it's inside a conditional
checking for allowed cases: either side being a pointer to a void type, or
the target types being compatible. Now the rule in 22.214.171.124#1 refers to
"qualified or unqualified version of void", which in ISO C terms does not
include _Atomic void, so those checks need to disallow _Atomic void. And
similarly, it's not enough for comp_target_types to match the unqualified
types when it also needs to match whether _Atomic is specified (for that,
comp_target_types should probably be changed - conditional expressions
have the same issue).
The above relate specifically to what's in the patch. The listed issues
should of course have testcases added. I still need to review it on the
basis of reviews of C11 for references to qualifiers or _Atomic, to see
what might be missing from the patch, and on the basis of reviews of the C
front end for handling of qualifiers, to see what places might need to
handle _Atomic specially; I'll do those reviews separately. As previously
noted, one thing missing is the _Atomic ( type-name ) syntax.
Joseph S. Myers