Question about existing tests for [dcl.stc]

[Aaron Ballman]

On Thu, May 22, 2014 at 9:04 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Mon, May 19, 2014 at 2:38 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Mon, May 19, 2014 at 5:32 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > You're citing the wrong section of the standard in your patch. The
>> > relevant
>> > wording is [dcl.stc]p1: "If a storage-class-specifier appears in a
>> > decl-specifier-seq, [...] the init-declarator-list of the declaration
>> > shall
>> > not be empty [...]."
>>
>> Ah, good to know. I made it down to p9 because we were declaring a
>> type instead of a member variable, but that makes sense.
>>
>> > Do we really need to change the code here? Is the new diagnostic better
>> > than
>> > the old one?
>>
>> Old diagnostic: ext-warn "'mutable' is not permitted on a declaration of a
>> type"
>> Proposed diagnostic: error "'mutable' can only be applied to member
>> variables"
>>
>> Honestly, either works for me in terms of wording.
>>
>> > Do we have a reason to promote this from ExtWarn to Error just
>> > for mutable and not for other storage class specifiers?
>>
>> I was going with error because the other mutable constraint violations
>> are also errors (including this existing one which I was reusing).
>> Also, it seems weird to treat this as an extension -- what does this
>> extension actually provide in terms of benefit for the mutable
>> keyword? What does a mutable type declaration even mean?
>
>
> It's weird that we allow this for other storage classes too. What does a
> static type declaration mean? =)
>
> No objection to the patch; I think I slightly prefer consistent treatment of
> 'mutable' over consistent treatment of storage classes on type declarations.
> But I wonder why we ExtWarn rather than Error'ing here in the other cases,
> and I'm slightly cautious about changing this without knowing the history.

I'm uncertain of the history, but for context:

// C++ [dcl.stc]p1:
//   If a storage-class-specifier appears in a decl-specifier-seq, [...] the
//   init-declarator-list of the declaration shall not be empty.
// C++ [dcl.fct.spec]p1:
//   If a cv-qualifier appears in a decl-specifier-seq, the
//   init-declarator-list of the declaration shall not be empty.
//
// Spurious qualifiers here appear to be valid in C.
unsigned DiagID = diag::warn_standalone_specifier;
if (getLangOpts().CPlusPlus)
  DiagID = diag::ext_standalone_specifier;

So I think we treat it as an extension because of C. But that makes no
sense for mutable anyway, which is why I am overriding that in my
patch and turning it into an error.

I've attached a revised patch that updates the comments to reflect my
understanding of why this is an error instead of an ExtWarn.

~Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mutable2.patch
Type: application/octet-stream
Size: 2032 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140523/6658056d/attachment.obj>