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

[Sebastian Redl]

Tim Northover wrote:
> I've now got the access control code to the point where I think it implements 
> a reasonably self-contained section of the standard, and I'm wondering what I 
> should do next.
>
> At the moment the code is almost certainly too stringent -- it will disallow 
> valid C++ programs where that wouldn't happen before. However getting it to 
> the stage where the only change is to disallow invalid programs would lead to 
> a much larger patch (the main issue would be dealing with friends everywhere 
> needed) which I understand is discouraged.
>   
We don't even parse friends yet. So if the only valid programs rejected
are those that involve friends, that's fine. However, if the patch makes
Clang reject valid programs that don't involve friends, that's a
problem. Of course, if those programs are very obscure, you can still
commit.
> The current patch is at http://www.maths.ed.ac.uk/~s0677366/access_first.diff 
> again.
>   
Detailed review below.
> So essentially I'm willing to do whatever people feel is best:
> + Work on problems with what I've got now.
> + Extend it to do what it does more correctly before submitting.
>   
I'm not sure where the difference between the two is. Anyway, I think
you should bring it to the point where it doesn't reject any valid
programs and rejects all invalid programs, under the assumption that
friends don't exist. Then implement friends.

In particular, be sure to handle this:

class C {
  typedef int I;
  I f();
};
C::I f() { return 0; }

> Index: lib/Sema/SemaAccess.cpp
> ===================================================================
> --- lib/Sema/SemaAccess.cpp	(revision 69821)
> +++ lib/Sema/SemaAccess.cpp	(working copy)
>   
> +  // C++ [class.access.base]p1
> +  // Describes how access specifiers propagate through a heirarchy.
> +  BasePath::const_reverse_iterator i = Path.rbegin(), e = Path.rend();
> +  for(; i != e; ++i) {
>   
A typo in "heirarchy", and put the iterator declarations into the for.
Yes, it results in a very long for condition, but that's the way we have
it throughout the code base.

> +    if (BestPerm.Perm == AS_none
> +        || (FoundPerm.Perm != AS_none && FoundPerm.Perm < BestPerm.Perm))
> +      BestPerm = FoundPerm;
>   

I wonder if any ill effects would come from simply reordering the AS_*
constants to none,private,protected,public and writing this (and
probably other places) as a single comparison.

> +/// Determines whether a given context has privileged access to the members of a
> +/// container.
>   
This is not what the function does. What it does is, it looks up a
context that *has* such access in the context stack.

> +  for(DeclContext *OuterContext = InnerContext; 
> +      OuterContext; OuterContext = OuterContext->getParent()) {
> +    if (!OuterContext->isRecord())
> +      continue;
>   
Would it be worth it to abort the loop on encountering a namespace? Or
are namespaces likely to be so shallow that the repeated test hurts more
than it helps? I don't think there's any way to ever find out.
Intuitively, I'd say no, though. The assumption here is that most
programs are valid, so a privileged context is found before reaching
namespace level.

> +bool Sema::CheckMemberAccessLimited(const RecordType *Container,
> +                                    NamedDecl *MemberDecl,
> +                                    DeclContext *ExprContext,
> +                                    AccessResult &Permission) {
>   
Is there any particular reason why MemberDecl and ExprContext are
non-const here? There's actually a lot of pointers in your patch that, I
think, could be made to-const.

Keep up the good work.

Sebastian