Re: [PATCH 1/3] add -Wstruct-not-pod, -Wclass-is-pod, -Wmismatched-tags (PR 61339)

[Martin Sebor <msebor at gmail dot com>]

On 8/1/19 12:09 PM, Jason Merrill wrote:
> On 7/22/19 12:34 PM, Martin Sebor wrote:
> > Ping: https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00622.html
> >
> > On 7/8/19 3:58 PM, Martin Sebor wrote:
> > > The attached patch implements three new warnings:
> > >
> > >   *  -Wstruct-not-pod triggers for struct definitions that are not
> > >      POD structs,
> > >   *  -Wclass-is-pod triggers for class definitions that satisfy
> > >      the requirements on POD structs, and
> > >   *  -Wmismatched-tags that triggers for class and struct declarations
> > >      with class-key that doesn't match either their definition or
> > >      the first declaration (if no definition is provided).
> > >
> > > The implementation of -Wclass-is-pod and -Wstruct-not-pod is fairly
> > > straightforward but the -Wmismatched-tags solution is slightly unusual.
> > > It collects struct and class declarations first and diagnoses mismatches
> > > only after the whole tramslation unit has been processed.  This is so
> > > that the definition of a class can guide which declarations to diagnose
> > > no matter which come first.
>
> As Jonathan and I were saying in the connected discussion, the *pod 
> warnings enforce questionable coding guidelines, so I'd rather not have 
> them in the compiler.

What specifically do you consider questionable?

When defining a new class one has to decide whether to use 'struct'
and when to use 'class'.  Since there's no difference, adopting any
convention to help make a consistent choice seems like a natural
thing to do.  You could say (as I think someone in this thread did):
"make it 'class' when it has a ctor or dtor."  Someone else might
add: "or assignment."  Someone else still might go even further
and include "private members or bases or virtual functions."  Since
all the type definitions end up equivalent regardless of the class-key,
all these choices seem equally reasonable.  But all of them are also
arbitrary.  The only difference is that by being based on a common
(and at least historically, widely relied on) property, the POD
convention is less arbitrary.  That's probably why it's apparently
quite popular (as evident from the Stack Exchange thread).

> -Wmismatched-tags is useful to have, given the MSVC/clang situation, but 
> I wonder about memory consumption from all the record keeping.  Do you 
> have any overhead measurements?

I did think about the overhead but not knowing if the patch would
even be considered I didn't spend time optimizing it.

In an instrumented build of GCC with the patch I just did I have
collected the following stats for the number of elements in
the rec2loc hash table (for 998 files):

  rec2loc elements   locvec elements
  min:           0                 0
  max:       2,785             3,303
  mean:        537             1,043
  median:      526             1,080

The locvec data are based on totals for the whole hash table, so
in the worst case the hash_map has 2,785 elements, and the sum of
all locvec lengths in all those elements is 3,303.  Each rec2loc
element takes up 16 bytes, plus the size of the locvec data.

If my math is right (which doesn't happen too often) in the worst
case the bookkeeping overhead is 43KB for the hash_map plus on
the order of 140KB for the vectors (just doubling the number of
elements to account for capacity = 2 X size, times 24 for
the flag_func_loc_t element type).  So 183K in the worst case
in GCC.

There are a few ways to reduce that if it seems excessive.

One is by avoiding some waste in flag_func_loc_t which is

  pair<tag_types, pair<bool, pair<tree, location_t>>>

tree could come first and tag_types and the bool could share
space.  That should bring it down to 16 in LP64, for about
30% off the 183K.

Beyond that, we could change the algorithm to discard records
for prior declarations after the first definition has been seen
(and any mismatches diagnosed).

We could also only collect one record for each definition
in system headers rather than one for every declaration and
reference.

I'd have to play with it to see how much of a difference these
tweaks would make.

If we mainly care about memory usage when compiling GCC (as
opposed to third party code that would have to explicitly opt
in to the warning) then I'd expect to get by far the biggest
savings by cleaning up GCC code to get rid of the unnecessary
class-key.

Martin