From: "Joseph S. Myers" <joseph@codesourcery.com>
To: Andrew MacLeod <amacleod@redhat.com>
Cc: <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH 3/5] Atomic type qualifier - front end changes
Date: Fri, 09 Aug 2013 00:06:00 -0000 [thread overview]
Message-ID: <Pine.LNX.4.64.1308082338060.13573@digraph.polyomino.org.uk> (raw)
In-Reply-To: <51F7F448.2010901@redhat.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
next prev parent reply other threads:[~2013-08-09 0:06 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-26 17:18 [PATCH] Add atomic type qualifier Andrew MacLeod
2013-07-26 19:24 ` Andi Kleen
2013-07-26 19:34 ` Andrew MacLeod
2013-07-26 21:14 ` Andi Kleen
2013-07-26 22:29 ` Andrew MacLeod
2013-07-26 20:42 ` Hans-Peter Nilsson
2013-07-26 23:58 ` Joseph S. Myers
2013-07-27 1:15 ` Andrew MacLeod
2013-08-01 21:54 ` Andrew MacLeod
2013-07-28 21:15 ` Joseph S. Myers
2013-07-29 15:47 ` Andrew MacLeod
2013-07-29 16:12 ` Joseph S. Myers
2013-07-29 16:30 ` Andrew MacLeod
2013-07-29 16:48 ` Joseph S. Myers
2013-07-29 18:20 ` Andrew MacLeod
2013-07-29 23:24 ` Andrew MacLeod
2013-07-30 12:03 ` Joseph S. Myers
2013-07-30 12:38 ` Andrew MacLeod
2013-07-30 13:25 ` Joseph S. Myers
2013-07-30 13:58 ` Andrew MacLeod
2013-07-31 12:19 ` Andrew MacLeod
2013-07-29 16:24 ` Andrew MacLeod
2013-07-30 16:56 ` [PATCH 0/5] Atomic " Andrew MacLeod
2013-07-30 17:13 ` [PATCH 2/5] Atomic type qualifier - Add atomic type to tree node Andrew MacLeod
2013-07-30 17:14 ` [PATCH 1/5] Atomic type qualifier - Add type alignment override hook Andrew MacLeod
2013-07-30 17:14 ` [PATCH 3/5] Atomic type qualifier - front end changes Andrew MacLeod
2013-08-09 0:06 ` Joseph S. Myers [this message]
2013-07-30 17:14 ` [PATCH 4/5] Atomic type qualifier - Change built-in function types Andrew MacLeod
2013-07-30 17:32 ` [PATCH 5/5] Atomic type qualifier - Use atomic objects Andrew MacLeod
2013-08-02 19:22 ` [PATCH] - C11 expressions and stdatomic.h - Just for current state Andrew MacLeod
2013-08-07 23:06 ` Joseph S. Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.64.1308082338060.13573@digraph.polyomino.org.uk \
--to=joseph@codesourcery.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).