Flexible array fields in C++ classes.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Flexible array fields in C++ classes.

Enea Zaffanella
Hello.

When compiling the following C++ code
==========================================
struct S {
   int size;
   int vec[]; // This is OK.
};

class C {
public:
   int size;
   int vec[]; // This triggers diagnostic.
};
==========================================

clang complains about the flexible array 'vec' that is found at the end
of class C, whereby it accepts the very same thing if at the end of
struct S. This is different wrt g++ behavior, which accepts both cases.

$ clang++ -fsyntax-only flex.cc
flex.cc:9:7: error: field has incomplete type 'int []'
   int vec[]; // This triggers diagnostic.
       ^
1 diagnostic generated.

Assuming that the one above is not what was really meant,
as far as I can see, the relevant code is in SemaDecl.cpp
(in function Sema::ActOnFields()):
==========================================
     } else if (FDTy->isIncompleteArrayType() && i == NumFields - 1 &&
                Record && Record->isStruct()) {
       // Flexible array member.
       if (NumNamedMembers < 1) {
         Diag(FD->getLocation(), diag::err_flexible_array_empty_struct)
           << FD->getDeclName();
         FD->setInvalidDecl();
         EnclosingDecl->setInvalidDecl();
         continue;
       }
       // Okay, we have a legal flexible array member at the end of the
struct.
       if (Record)
         Record->setHasFlexibleArrayMember(true);
==========================================

The impression is that the if-guard condition
     Record->isStruct()
is an overkill and could be replaced by the weaker form
     (Record->isStruct() || Record->isClass())

Patch is attached.

Cheers,
Enea Zaffanella.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

flex_array_in_class.patch (602 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Flexible array fields in C++ classes.

Ivan Sorokin
Hi!

Enea Zaffanella wrote:
>
> The impression is that the if-guard condition
>     Record->isStruct()
> is an overkill and could be replaced by the weaker form
>     (Record->isStruct() || Record->isClass())
>
As I know in C++ the only difference between struct and class is that by
default in struct members (and bases) are public and in class -- private.

Has it a sense to look through every place where isStruct() exists and
decide whether isClass() should also be there?

I did this, but as I saw there is no such use of both of isStruct() and
isClass(). isStruct() and isClass() seems to be used in different
places. So I am a bit confused. Maybe some one else who has more
knowledge about internals of clang looks at this?

Sorry, if this message is useless.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Flexible array fields in C++ classes.

Douglas Gregor
In reply to this post by Enea Zaffanella

On Feb 2, 2010, at 7:10 AM, Enea Zaffanella wrote:

> Hello.
>
> When compiling the following C++ code
> ==========================================
> struct S {
>  int size;
>  int vec[]; // This is OK.
> };
>
> class C {
> public:
>  int size;
>  int vec[]; // This triggers diagnostic.
> };
> ==========================================
>
> clang complains about the flexible array 'vec' that is found at the end of class C, whereby it accepts the very same thing if at the end of struct S. This is different wrt g++ behavior, which accepts both cases.
>
> $ clang++ -fsyntax-only flex.cc
> flex.cc:9:7: error: field has incomplete type 'int []'
>  int vec[]; // This triggers diagnostic.
>      ^
> 1 diagnostic generated.

Frankly, I'd rather reject both cases in C++. Flexible arrays collide with C++ in a few places, and we need to find those places and address them before enabling this extension. I have the same view toward variable-length arrays in C++ (http://llvm.org/bugs/show_bug.cgi?id=5678): we *can* have the extension, but we need to complain and we need to deal with the various corner cases before enabling the extension. If we aren't strict about this, we'll end up where GCC is now, with a bunch of half-baked extensions.

For example, I wrote this little test:

struct S {
  int size;
  int vec[];
};

struct S2 {
  S s;
  int x;
};

struct T : S { int x; };

template<typename T>
struct X { T t; int x; };

X<S> xs;

Even under -Wall, g++ doesn't complain about anything here... it doesn't complain about the definition of S2, which has a non-static data member ('x') following a non-static data member ('s') that has a flexible array member, nor does it complain about derivation from a class with a flexible array member, which is the moral equivalent of the previous problem. Clang does better (it warns about the first case, along with case where this happens in templates), but misses derivation.

So, Clang is already in the "half-baked" category with this extension to C++. We should either finish the extension or ban it in C++, but we shouldn't extend it 1% and then leave it there.

        - Doug

> Assuming that the one above is not what was really meant,
> as far as I can see, the relevant code is in SemaDecl.cpp
> (in function Sema::ActOnFields()):
> ==========================================
>    } else if (FDTy->isIncompleteArrayType() && i == NumFields - 1 &&
>               Record && Record->isStruct()) {
>      // Flexible array member.
>      if (NumNamedMembers < 1) {
>        Diag(FD->getLocation(), diag::err_flexible_array_empty_struct)
>          << FD->getDeclName();
>        FD->setInvalidDecl();
>        EnclosingDecl->setInvalidDecl();
>        continue;
>      }
>      // Okay, we have a legal flexible array member at the end of the struct.
>      if (Record)
>        Record->setHasFlexibleArrayMember(true);
> ==========================================
>
> The impression is that the if-guard condition
>    Record->isStruct()
> is an overkill and could be replaced by the weaker form
>    (Record->isStruct() || Record->isClass())
>
> Patch is attached.
>
> Cheers,
> Enea Zaffanella.
> <flex_array_in_class.patch>_______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev