[cfe-dev] Beginnings of C++ access control

[Sebastian Redl]

Tim Northover wrote:
> Hello,
>
> I've been working on implementing C++ access control in the Sema stage, and 
> now have a very preliminary patch (hopefully attached, otherwise inline in a 
> followup -- still not sure if the list supports attachments).
>   
Hi,

Welcome to Clang. It's always nice to see new people join.
Your patch came through fine. Comments are below. One general comment:
please convert all tabs to spaces.

> Index: test/SemaCXX/access.cpp
> ===================================================================
> --- test/SemaCXX/access.cpp	(revision 69439)
> +++ test/SemaCXX/access.cpp	(working copy)
>   
> +  x = bprota.x; // expected-error {{member 'A::x' is protected}}
> +  x = bprota.y; // expected-error {{member 'A::y' is protected}}
> +  x = bprota.z; // expected-error {{member 'A::z' is inaccessible}}
> +
> +  x = bpriva.x; // expected-error {{member 'A::x' is private}}
> +  x = bpriva.y; // expected-error {{member 'A::y' is private}}
> +  x = bpriva.z; // expected-error {{member 'A::z' is inaccessible}}
>   
The distinction between private/protected and inaccessible is confusing.
When is which used, and can we get rid of 'inaccessible'?
After looking at the rest of the code, I can see where the problem with
that is. But I think this suggests a problem with the overall structure
of the code. I think your checker functions should be able to report the
class where the member was last accessible, so that we can formulate an
error like, "member A::y is private in 'class B'". It would be even
nicer if the message could mention that this is because B inherits
privately from A.

> +  x = bprota.x; // expected-error {{member 'A::x' is protected}}
> +  x = bprota.y; // expected-error {{member 'A::y' is protected}}
> +  x = bprota.z; // expected-error {{member 'A::z' is inaccessible}}
> +
> +  x = bpriva.x; // expected-error {{member 'A::x' is private}}
> +  x = bpriva.y; // expected-error {{member 'A::y' is private}}
> +  x = bpriva.z; // expected-error {{member 'A::z' is inaccessible}}
>   
> +/// Finds out whether member is a public/private/protected part of the accessor.
> +AccessSpecifier
> +Sema::AccessMemberFromRecord(NamedDecl *MemberDecl, 
> +			     const RecordType *AccessorType) {
>   

I don't like the name of this function; it confuses me. Is AccessorType
the context of the function that accesses the field, or is it the type
that's supposed to contain the field? The first is suggested by the
name, but the second by the comment.
OK, looking at the code below, I can see it's the type through which the
field is accessed. I think this could be clearer. Call the function
"AccessMemberThroughRecord(NamedDecl *Member, const RecordType *Container)"

> +
> +  RecordDecl *BaseRecord = 0;
> +  if (MemberDecl->getDeclContext()->isRecord())
> +    BaseRecord = cast<RecordDecl>(MemberDecl->getDeclContext());
>   

RecordDecl *BaseRecord = dyn_cast<RecordDecl>(MemberDecl->getDeclContext());

> +  if (!BaseRecord || BaseRecord->isAnonymousStructOrUnion()) {
> +    // Either not a record: it's a union or it's anonymous.
>   

This is not a good comment. It explains the what of the condition above,
which is not very helpful (I can read code), but it doesn't explain the
why, which would be.

> +    for (DeclContext *DC = MemberDecl->getDeclContext();
> +	 DC;
> +	 DC = DC->getParent()) {
>   

Minor stylistic issue: we usually would keep condition and incrementer
on the same line if they fit. (That is, merge the second and third line
of this snippet.)

> +    AccessSpecifier Perm = PropagateMemberAccess(IntrinsicPerm, Path);
>   

This usage of PropagateMemberAccess suggests that the name isn't ideal
either. Perhaps something like "GetAccessThroughPath" would be more
appropriate.

> +    if (BestPerm == AS_none)
> +      BestPerm = Perm;
> +    else if (Perm != AS_none && Perm < BestPerm)
> +      BestPerm = Perm;
>   

Merge the two.

> +  // [class.access.base]:p5
> +  // A member m is accessible at the point R when named in class N if:
>   

Please observe the standard quotation style; there's an automated tool
that collects the quotes. The style is:

std sectionparagraph

e.g.
C++ [class.access.base]p5
or
C++ 5.16p4


All in all, this is pretty good work. Thanks for tackling this beast.

Sebastian