[cfe-commits] [PATCH][Review request] - Anonymous union/struct redesign

[Francois Pichet]

Hi John

Thank you for the quick review, here is an updated patch
See inline comments.


On Wed, Nov 17, 2010 at 3:19 AM, John McCall <rjmccall at apple.com> wrote:
>
> This can be cast<FieldDecl>;  the final member will always be a FieldDecl.
>
> Please add an accessor to get the base declaration, i.e. *chain_begin().  It
> might also help to have asserting accessors to grab the first member as a
> FieldDecl or VarDecl.
>

Accessor to get the base declaration for what? I didn't feel the need of it.

> @@ -1920,6 +1934,8 @@
>         }
>       } else if (isa<AccessSpecDecl>(*Mem)) {
>         // Any access specifier is fine.
> +      } else if (isa<IndirectFieldDecl>(*Mem)) {
> +        // Any access specifier is fine.
>
> Fix the comment, please.

I actually removed that code. It is not necessary.

>
> +++ lib/Sema/SemaDeclCXX.cpp    (working copy)
> @@ -1067,11 +1067,16 @@
>     FieldDecl *Member = 0;
>     DeclContext::lookup_result Result
>       = ClassDecl->lookup(MemberOrBase);
> -    if (Result.first != Result.second)
> +    if (Result.first != Result.second) {
>       Member = dyn_cast<FieldDecl>(*Result.first);
> +
> +         // Handle anonymous union case.
> +         if (!Member)
> +      if (IndirectFieldDecl* IndirectField
> +          = dyn_cast<IndirectFieldDecl>(*Result.first))
> +        Member = IndirectField->getAnonField();
> +  }
>
> -    // FIXME: Handle members of an anonymous union.
> -
>
> I think this would be cleaner if we built the member initializer with an
> IndirectFieldDecl*, but we can fix that in a later patch.

Yes later patch, this require changes in SemaInit.cpp

>
> @@ -2599,15 +2604,21 @@
>     //   In addition, if class T has a user-declared constructor (12.1), every
>     //   non-static data member of class T shall have a name different from T.
>     for (DeclContext::lookup_result R = Record->lookup(Record->getDeclName());
> -         R.first != R.second; ++R.first)
> +         R.first != R.second; ++R.first) {
>       if (FieldDecl *Field = dyn_cast<FieldDecl>(*R.first)) {
> -        if (Record->hasUserDeclaredConstructor() ||
> -            !Field->getDeclContext()->Equals(Record)) {
> +        if (Record->hasUserDeclaredConstructor()) {
> +         Diag(Field->getLocation(), diag::err_member_name_of_class)
> +            << Field->getDeclName();
> +          break;
> +        }
> +      }
> +      else if (IndirectFieldDecl *Field =
> +                    dyn_cast<IndirectFieldDecl>(*R.first)) {
>         Diag(Field->getLocation(), diag::err_member_name_of_class)
>           << Field->getDeclName();
>
> This can be
>  NamedDecl *D = *R.first;
>  if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D)) {
>    ...
>  }

What is needed is:
      NamedDecl *D = *R.first;
      if ((isa<FieldDecl>(D) && Record->hasUserDeclaredConstructor()) ||
          isa<IndirectFieldDecl>(D)) {



> +++ lib/Sema/SemaExpr.cpp       (working copy)
>
> +VarDecl *Sema::BuildAnonymousStructUnionMemberPath(IndirectFieldDecl *Field,
>                                    llvm::SmallVectorImpl<FieldDecl *> &Path) {
>
> I'm not convinced this method needs to exist anymore, but if it does:

Why not? this function is called at 2 places. That code shall not be duplicated.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: anonymous2.patch
Type: application/octet-stream
Size: 26906 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101120/5a9d19f6/attachment.obj>