This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/3] add support for POD struct convention (PR 61339)


On 7/12/19 9:36 AM, Jason Merrill wrote:
On Fri, Jul 12, 2019 at 11:14 AM Martin Sebor <msebor@gmail.com> wrote:

On 7/12/19 5:42 AM, Jonathan Wakely wrote:
On 12/07/19 10:24 +0200, Jakub Jelinek wrote:
On Mon, Jul 08, 2019 at 03:56:51PM -0600, Martin Sebor wrote:
A couple of GCC's Coding Conventions call to

   1) Use the struct keyword for plain old data (POD) types.
      https://www.gnu.org/software/gcc/codingrationale.html#struct

and

   2) Use the class keyword for non-POD types.
      https://www.gnu.org/software/gcc/codingconventions.html#Class_Use

This is a coding convention that has been added without any discussion
whatsoever on that, maybe it was some Google internal coding
convention or
something, do we really want to enforce it rather than discuss
and decide what we actually want?

With my limited C++ knowledge, the main distinction between struct and
class
when both can appear interchangeably is that struct defaults to public:

The default applies to class members and base classes, but that's the
*only* distinction.

and class defaults to private:, and I think it is best to use those that
way, rather than having tons of class ... { public: ... } everywhere.

There are many C++ class boolean properties, rather than just
POD vs. non-POD and we could pick any of them instead of this
particular one
for the struct vs. class distinction if we wanted to enforce it, but why?

I'm also unconvinced that POD is a useful distinction. A class might
be a POD today, but then we decide to add a default constructor to it
(or if/when we move to C++11, add default member initializers to it).
Does that mean we need to replace struct with class to follow this
convention?

Or we might decide to add a std::string member to it, which stops it
being a POD. Should every reference to it be changed from struct to
class?

Right, that's a downside of such a convention.  It can be mitigated
(but not avoided completely) by eschewing the class-key in references
to the type if it's unambiguous.  The warning suggests to drop
the class-key when it's possible.  I didn't follow that suggestion
in the cleanup patch only out of concern that people used to seeing
the 'class' or 'struct' there might be bothered by its removal.

At the same time, adding a std::string member to A POD can have
serious ripple effects in a code base that assumes the struct is
a POD.  -Wclass-memaccess finds a subset of those but not nearly
all of them (I'd love to see a warning that detected more).

Personally I think "no user-defined constructors" might be a more
useful distinction than POD. This is not a POD (because of the
std::string member) but I don't see why it should be a class not a
struct:

struct A {
   std::string name;
   int id;
};

It looks like a fine struct to me :)

But the answer depends on the goal of the convention.  If it is
to make it clear to users of A whether it can be used in a context
that expects a "POD" (such as some GCC containers) then A would
have to be a class.  Ideally, the POD requirement would be enforced
by smarter warnings and/or by type traits and static assertions but
not all projects make use of those (yet).

If a container has requirements of an element class, it should enforce
them with STATIC_ASSERT.  Trying to express this distinction with
'struct' vs 'class' seems obscure and fragile; that's an attribute of
a class that can't be checked by the actual code.

You're preaching to the choir here :)  See pr90904 for an example
where GCC's own auto_vec corrupts memory when used with a non-POD
type because it doesn't handle assignability correctly.  It doesn't
have a static assert to enforce it and I'm not sure it needs to
-- I think it should be fixed instead.

Richard's suggestion in the bug, one that I took, was to use vec
instead of auto_vec.  But vec itself is not safely assignable or
even copy constructible, so strictly speaking, it shouldn't be
used in containers that have that requirement (like hash_map).
The bug I ran into with hash_map then was pr90923 -- another
memory corruption that resulted in miscompilation due to hash_map
not being quire prepared to correctly handle non-POD types.

I'm sure these kinds of bugs aren't unique to GCC but are common
in any non-trivial code base that's migrating from C to C++.  And
that's where I think a convention like GCC's might be helpful.  I
suspect Lawrence may have introduced the POD struct guideline just
because he was anticipating these problems during GCC's migration
to C++.

Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]