Re: [PATCH 3/5] Atomic type qualifier - front end changes

["Joseph S. Myers" <joseph@codesourcery.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

        case cdk_pointer:
          {
            /* Merge any constancy or volatility into the target type
               for the pointer.  */

            if (pedantic && TREE_CODE (type) == FUNCTION_TYPE
                && type_quals)
              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 
selected).

* 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 6.5.16.1#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
joseph@codesourcery.com