Access specifiers and anonymous unions

class S {
  union {
    int a;
    double b;
  };
};

I've just verified that in the example above clang changes the access
specifier of the two fields of anonymous union to inherit the access
specificier of anonymous union.

Apart that this is contrary to 9.5p3 "An anonymous union shall not have
private or protected members (clause 11)" I might imagine this is done
for access checking, but as I'd guess that access checking is done
traversing and checking *all* the nesting records (also anonymous ones)
the union fields would be unaccessible from outside class S also if
correctly marked as public.

Why this is done? Is it a bug?

Your guess is wrong; we assume that the decl's access specifier correctly
describes the access of the member when considered as a member of its
declaring class. There is no value in remembering the access of the decl
within the anonymous union, first, because it is impossible to access
anything from the context of the anonymous union (as opposed to
from its enclosing context) after it is parsed, and second, because
(as you've rightly observed) that access is always public.

Is there a value to remembering the original access specifier, i.e. public,
that I'm not seeing? I would much prefer not to change the current behavior,
but we could certainly provide a method that reports the "direct" access
specifier, i.e. public if the DC is an anonymous struct or union.

John.

I might imagine this is done
for access checking, but as I'd guess that access checking is done
traversing and checking *all* the nesting records (also anonymous ones)
the union fields would be unaccessible from outside class S also if
correctly marked as public.

Your guess is wrong; we assume that the decl's access specifier correctly
describes the access of the member when considered as a member of its
declaring class. There is no value in remembering the access of the decl
within the anonymous union, first, because it is impossible to access
anything from the context of the anonymous union (as opposed to
from its enclosing context) after it is parsed, and second, because
(as you've rightly observed) that access is always public.

I'm not sure to understand the difference you see between

class s {
  struct {
    int a;
    int b;
  };
};

and

class q {
  struct {
    int a;
    int b;
  } c;
};

and why you treat the former as a special case, but that apart it's not
a problem in either way once there is the possibility to know access
specifier written in source.

Is there a value to remembering the original access specifier, i.e. public,
that I'm not seeing? I would much prefer not to change the current behavior,
but we could certainly provide a method that reports the "direct" access
specifier, i.e. public if the DC is an anonymous struct or union.

The value is for pretty printer like applications and for source
checkers. However to add an AccessSpecifierAsWritten flag inside the
decl is not the ideal solution, it can't represent something like this:

class s {
private:
  int a;
};

(undistiguishable from class s { int a; })

or

class s {
public:
private:
  int a;
};

To represent correctly the above we might decide to have a node for
access specifiers at the same level of other decls.

I might imagine this is done

for access checking, but as I’d guess that access checking is done

traversing and checking all the nesting records (also anonymous ones)

the union fields would be unaccessible from outside class S also if

correctly marked as public.

Your guess is wrong; we assume that the decl’s access specifier correctly

describes the access of the member when considered as a member of its

declaring class. There is no value in remembering the access of the decl

within the anonymous union, first, because it is impossible to access

anything from the context of the anonymous union (as opposed to

from its enclosing context) after it is parsed, and second, because

(as you’ve rightly observed) that access is always public.

I’m not sure to understand the difference you see between

class s {
struct {
int a;
int b;
};
};

and

class q {
struct {
int a;
int b;
} c;
};

and why you treat the former as a special case, but that apart it’s not
a problem in either way once there is the possibility to know access
specifier written in source.

Because it is a special case. The first case is an anonymous union; the
second case is just a field of anonymous type. In the first case, ‘a’ and ‘b’
are visible as members of ‘s’; in the second case they are not.

This is C++ [class.union]p4:
A union for which objects or pointers are declared is not an anonymous union.

Is there a value to remembering the original access specifier, i.e. public,

that I’m not seeing? I would much prefer not to change the current behavior,

but we could certainly provide a method that reports the “direct” access

specifier, i.e. public if the DC is an anonymous struct or union.

The value is for pretty printer like applications and for source
checkers. However to add an AccessSpecifierAsWritten flag inside the
decl is not the ideal solution, it can’t represent something like this:

class s {
private:
int a;
};

(undistiguishable from class s { int a; })

or

class s {
public:
private:
int a;
};

To represent correctly the above we might decide to have a node for
access specifiers at the same level of other decls.

Yes, I’ve been wanting something like this for some time.

John.

Would it be OK if we derive from Decl a new class modeling syntactic
access specifiers occurring in the list of member specifiers of a C++
class definition?

What about the name AccessSpecDecl ?

Deriving from Decl already provides space for storing the access
specifier (cannot be AS_none) and the corresponding source location.
While at it, should we also provide a source location for the ':' ?

Am I right if I say that all code visiting CXXRecordDecl as a
declaration context will automatically ignore this new class (which is
actually a good thing)?

We should only consider them in dumping/pretty printing code.

Cheers,
Abramo

To represent correctly the above we might decide to have a node for
access specifiers at the same level of other decls.

Yes, I've been wanting something like this for some time.

Would it be OK if we derive from Decl a new class modeling syntactic
access specifiers occurring in the list of member specifiers of a C++
class definition?

What about the name AccessSpecDecl ?

Decl actually has a lot of space overhead, and if we put these in the decl chain
we have to instantiate them, etc. If we want to be really space-conserving, we
could just add an array of little structs to CXXRecordDecl::DefinitionData.
On the other hand, that would make them much more difficult to use; I'm torn.

Deriving from Decl already provides space for storing the access
specifier (cannot be AS_none) and the corresponding source location.
While at it, should we also provide a source location for the ':' ?

Probably not necessary.

Am I right if I say that all code visiting CXXRecordDecl as a
declaration context will automatically ignore this new class (which is
actually a good thing)?

Well, if they were Decls, lookup would ignore them because they wouldn't
be NamedDecls, but they'd still show up in the lexical decl chain. That's
probably fine.

John.

I've attached the patch that introduces AccessSpecDecl for your approval.

Of course it passes all tests.

AccessSpec.patch (13.1 KB)

Ping.

I apologize to annoy you, but we need to receive a bit of direction to
continue our work for clang. We have a other patchs waiting to be submitted.

Sorry, it's been a busy week here, and I wanted to talk this over with a few people
who've been even busier.

John.

After some discussion, we decided that the potential memory savings of avoiding
making Decls here was Not Worth It. We can revisit that decision if shown that
it matters.

Please commit, and thanks for being patient.

John.

Commited in r105525.

Thanks for your review.