[PATCH] Fix few build warnings with LLVM toolchain

[PATCH] Fix few build warnings with LLVM toolchain

[David CARLIER]

[-- Attachment #1: Type: text/plain, Size: 65 bytes --]

Hi,

Here a tiny patch to fix few build warnings.

Kind regards.

[-- Attachment #2: patch-class2struct-few-fix.diff --]
[-- Type: text/x-patch, Size: 2098 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 271684)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2019-05-28  David Carlier <devnexen@gmail.com>
+
+        * coretypes.h: Fix build warning, chaing few classes to struct.
+
 2019-05-27  Jakub Jelinek  <jakub@redhat.com>
 
 	* gimplify.c (gimplify_scan_omp_clauses): Allow lastprivate conditional
Index: gcc/coretypes.h
===================================================================
--- gcc/coretypes.h	(revision 271684)
+++ gcc/coretypes.h	(working copy)
@@ -65,7 +65,7 @@ template<typename> class opt_mode;
 typedef opt_mode<scalar_mode> opt_scalar_mode;
 typedef opt_mode<scalar_int_mode> opt_scalar_int_mode;
 typedef opt_mode<scalar_float_mode> opt_scalar_float_mode;
-template<typename> class pod_mode;
+template<typename> struct pod_mode;
 typedef pod_mode<scalar_mode> scalar_mode_pod;
 typedef pod_mode<scalar_int_mode> scalar_int_mode_pod;
 typedef pod_mode<fixed_size_mode> fixed_size_mode_pod;
@@ -73,7 +73,7 @@ typedef pod_mode<fixed_size_mode> fixed_
 /* Subclasses of rtx_def, using indentation to show the class
    hierarchy, along with the relevant invariant.
    Where possible, keep this list in the same order as in rtl.def.  */
-class rtx_def;
+struct rtx_def;
   class rtx_expr_list;           /* GET_CODE (X) == EXPR_LIST */
   class rtx_insn_list;           /* GET_CODE (X) == INSN_LIST */
   class rtx_sequence;            /* GET_CODE (X) == SEQUENCE */
@@ -138,7 +138,7 @@ struct gomp_teams;
 /* Subclasses of symtab_node, using indentation to show the class
    hierarchy.  */
 
-class symtab_node;
+struct symtab_node;
   struct cgraph_node;
   class varpool_node;
 
Index: gcc/hash-table.h
===================================================================
--- gcc/hash-table.h	(revision 271684)
+++ gcc/hash-table.h	(working copy)
@@ -347,7 +347,7 @@ hash_table_mod2 (hashval_t hash, unsigne
   return 1 + mul_mod (hash, p->prime - 2, p->inv_m2, p->shift);
 }
 
-class mem_usage;
+struct mem_usage;
 
 /* User-facing hash table type.
 

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Segher Boessenkool]

On Tue, May 28, 2019 at 09:31:18AM +0000, David CARLIER wrote:
> Here a tiny patch to fix few build warnings.

Please mention what the warning _is_, and why it is correct / why it is
a good idea to make these changes.


Segher

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Martin Liška]

On 5/28/19 11:31 AM, David CARLIER wrote:
> Hi,
> 
> Here a tiny patch to fix few build warnings.
> 
> Kind regards.
> 

Hi.

Well, I see a lot of these struct/class discrepancies when building GCC with LLVM.
Question is whether it worth changing?

Martin

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Jakub Jelinek]

On Tue, May 28, 2019 at 12:24:12PM +0200, Martin Liška wrote:
> Well, I see a lot of these struct/class discrepancies when building GCC with LLVM.
> Question is whether it worth changing?

No, the warning is just wrong.  C++ clearly states what the struct and class
keyword mean and it is just fine to mix them between declarations and
definitions.

	Jakub

Fwd: [PATCH] Fix few build warnings with LLVM toolchain

[David CARLIER]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

---------- Forwarded message ---------
From: David CARLIER <devnexen@gmail.com>
Date: Tue, 28 May 2019 at 10:16
Subject: Re: [PATCH] Fix few build warnings with LLVM toolchain
To: Segher Boessenkool <segher@kernel.crashing.org>


All right, here an updated version, hope it looks better.

Thanks.

On Tue, 28 May 2019 at 10:09, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Tue, May 28, 2019 at 09:31:18AM +0000, David CARLIER wrote:
> > Here a tiny patch to fix few build warnings.
>
> Please mention what the warning _is_, and why it is correct / why it is
> a good idea to make these changes.
>
>
> Segher

[-- Attachment #2: patch-class2struct-few-fix.diff --]
[-- Type: application/x-patch, Size: 2710 bytes --]

Re: [PATCH] Fix few build warnings with LLVM toolchain

[David CARLIER]

Yes I sort of agree it is pretty harmless even though I cannot tell
for Microsoft toolchain issue here.

On Tue, 28 May 2019 at 10:27, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, May 28, 2019 at 12:24:12PM +0200, Martin Liška wrote:
> > Well, I see a lot of these struct/class discrepancies when building GCC with LLVM.
> > Question is whether it worth changing?
>
> No, the warning is just wrong.  C++ clearly states what the struct and class
> keyword mean and it is just fine to mix them between declarations and
> definitions.
>
>         Jakub

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Martin Sebor]

On 5/28/19 4:24 AM, Martin Liška wrote:
> On 5/28/19 11:31 AM, David CARLIER wrote:
>> Hi,
>>
>> Here a tiny patch to fix few build warnings.
>>
>> Kind regards.
>>
> 
> Hi.
> 
> Well, I see a lot of these struct/class discrepancies when building GCC with LLVM.
> Question is whether it worth changing?

I think it's nice for these to be spelled consistently and no benefit
to mixing and matching them.  If it cleans up common warnings I see
no reason not to make the change.

FWIW, it's also a common convention to use struct for PODs and class
for types with user-defined ctors, and even if GCC doesn't subscribe
to it, make a change in support of it is an improvement independent
of the Visual C++ warning.  (As might be adding such a warning to
GCC to help enforce the convention on projects that do follow it.)

Martin

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Eric Gallager]

On 5/28/19, Martin Sebor <msebor@gmail.com> wrote:
> On 5/28/19 4:24 AM, Martin Liška wrote:
>> On 5/28/19 11:31 AM, David CARLIER wrote:
>>> Hi,
>>>
>>> Here a tiny patch to fix few build warnings.
>>>
>>> Kind regards.
>>>
>>
>> Hi.
>>
>> Well, I see a lot of these struct/class discrepancies when building GCC
>> with LLVM.
>> Question is whether it worth changing?
>
> I think it's nice for these to be spelled consistently and no benefit
> to mixing and matching them.  If it cleans up common warnings I see
> no reason not to make the change.
>
> FWIW, it's also a common convention to use struct for PODs and class
> for types with user-defined ctors, and even if GCC doesn't subscribe
> to it, make a change in support of it is an improvement independent
> of the Visual C++ warning.  (As might be adding such a warning to
> GCC to help enforce the convention on projects that do follow it.)
>
> Martin
>

The bug for discussing -Wmismatched-tags is bug 61339, FWIW:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61339

Re: Fwd: [PATCH] Fix few build warnings with LLVM toolchain

[Segher Boessenkool]

On Tue, May 28, 2019 at 10:28:07AM +0000, David CARLIER wrote:
> ---------- Forwarded message ---------
> From: David CARLIER <devnexen@gmail.com>
> Date: Tue, 28 May 2019 at 10:16
> Subject: Re: [PATCH] Fix few build warnings with LLVM toolchain
> To: Segher Boessenkool <segher@kernel.crashing.org>
> 
> 
> All right, here an updated version, hope it looks better.

Please don't top-post, and don't use base64 either?  Making it hard for
people to reply to your patch submission does not help you.


Segher

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Martin Sebor]

On 5/28/19 10:31 AM, Martin Sebor wrote:
> On 5/28/19 4:24 AM, Martin Liška wrote:
>> On 5/28/19 11:31 AM, David CARLIER wrote:
>>> Hi,
>>>
>>> Here a tiny patch to fix few build warnings.
>>>
>>> Kind regards.
>>>
>>
>> Hi.
>>
>> Well, I see a lot of these struct/class discrepancies when building 
>> GCC with LLVM.
>> Question is whether it worth changing?
> 
> I think it's nice for these to be spelled consistently and no benefit
> to mixing and matching them.  If it cleans up common warnings I see
> no reason not to make the change.
> 
> FWIW, it's also a common convention to use struct for PODs and class
> for types with user-defined ctors, and even if GCC doesn't subscribe
> to it, make a change in support of it is an improvement independent
> of the Visual C++ warning.  (As might be adding such a warning to
> GCC to help enforce the convention on projects that do follow it.)

Jeff reminded me in a code review the other day that GCC does
have a guideline for defining POD structs with the keyword
"struct" and classes with ctors/dtors using "class":

   https://gcc.gnu.org/codingconventions.html#Struct_Use

I quickly prototyped a warning to see how closely GCC follows
this convention.  The result shows that out of just under 800
structs and classes defined in GCC sources some 200 use
the keyword 'struct' despite having a ctor or dtor, or about
25%.  So as is the case with most other conventions, without
a tool to help remind us they exist they are unlikely to be
followed with enough consistency to be worth putting in place
to begin with.

Martin

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Segher Boessenkool]

On Fri, Jun 28, 2019 at 08:55:00AM -0600, Martin Sebor wrote:
> Jeff reminded me in a code review the other day that GCC does
> have a guideline for defining POD structs with the keyword
> "struct" and classes with ctors/dtors using "class":
> 
>   https://gcc.gnu.org/codingconventions.html#Struct_Use
> 
> I quickly prototyped a warning to see how closely GCC follows
> this convention.  The result shows that out of just under 800
> structs and classes defined in GCC sources some 200 use
> the keyword 'struct' despite having a ctor or dtor, or about
> 25%.  So as is the case with most other conventions, without
> a tool to help remind us they exist they are unlikely to be
> followed with enough consistency to be worth putting in place
> to begin with.

The goal is not to have the rules adhered to.  The goal is to have
a more readable / maintainable / etc. codebase.

And even *if* you evaluate success of a rule just by how well it is
followed, you'll need to look at separate areas of GCC separately,
not just look at one gross number, one randomly defined statistic.


Segher

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Jeff Law]

On 6/28/19 8:55 AM, Martin Sebor wrote:
> On 5/28/19 10:31 AM, Martin Sebor wrote:
>> On 5/28/19 4:24 AM, Martin Liška wrote:
>>> On 5/28/19 11:31 AM, David CARLIER wrote:
>>>> Hi,
>>>>
>>>> Here a tiny patch to fix few build warnings.
>>>>
>>>> Kind regards.
>>>>
>>>
>>> Hi.
>>>
>>> Well, I see a lot of these struct/class discrepancies when building
>>> GCC with LLVM.
>>> Question is whether it worth changing?
>>
>> I think it's nice for these to be spelled consistently and no benefit
>> to mixing and matching them.  If it cleans up common warnings I see
>> no reason not to make the change.
>>
>> FWIW, it's also a common convention to use struct for PODs and class
>> for types with user-defined ctors, and even if GCC doesn't subscribe
>> to it, make a change in support of it is an improvement independent
>> of the Visual C++ warning.  (As might be adding such a warning to
>> GCC to help enforce the convention on projects that do follow it.)
> 
> Jeff reminded me in a code review the other day that GCC does
> have a guideline for defining POD structs with the keyword
> "struct" and classes with ctors/dtors using "class":
> 
>   https://gcc.gnu.org/codingconventions.html#Struct_Use
> 
> I quickly prototyped a warning to see how closely GCC follows
> this convention.  The result shows that out of just under 800
> structs and classes defined in GCC sources some 200 use
> the keyword 'struct' despite having a ctor or dtor, or about
> 25%.  So as is the case with most other conventions, without
> a tool to help remind us they exist they are unlikely to be
> followed with enough consistency to be worth putting in place
> to begin with.
This kind of response just makes you look combative.  We have a
convention and we should follow it.  Please do so for your code.

If we can cleanly implement this kind of diagnostic, then I would
certainly suggest we do so, fix our codebase and turn it on by default
for GCC builds, regardless of its status in -Wall.

jeff

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Martin Sebor]

On 6/28/19 10:59 AM, Jeff Law wrote:
> On 6/28/19 8:55 AM, Martin Sebor wrote:
>> On 5/28/19 10:31 AM, Martin Sebor wrote:
>>> On 5/28/19 4:24 AM, Martin Liška wrote:
>>>> On 5/28/19 11:31 AM, David CARLIER wrote:
>>>>> Hi,
>>>>>
>>>>> Here a tiny patch to fix few build warnings.
>>>>>
>>>>> Kind regards.
>>>>>
>>>>
>>>> Hi.
>>>>
>>>> Well, I see a lot of these struct/class discrepancies when building
>>>> GCC with LLVM.
>>>> Question is whether it worth changing?
>>>
>>> I think it's nice for these to be spelled consistently and no benefit
>>> to mixing and matching them.  If it cleans up common warnings I see
>>> no reason not to make the change.
>>>
>>> FWIW, it's also a common convention to use struct for PODs and class
>>> for types with user-defined ctors, and even if GCC doesn't subscribe
>>> to it, make a change in support of it is an improvement independent
>>> of the Visual C++ warning.  (As might be adding such a warning to
>>> GCC to help enforce the convention on projects that do follow it.)
>>
>> Jeff reminded me in a code review the other day that GCC does
>> have a guideline for defining POD structs with the keyword
>> "struct" and classes with ctors/dtors using "class":
>>
>>    https://gcc.gnu.org/codingconventions.html#Struct_Use
>>
>> I quickly prototyped a warning to see how closely GCC follows
>> this convention.  The result shows that out of just under 800
>> structs and classes defined in GCC sources some 200 use
>> the keyword 'struct' despite having a ctor or dtor, or about
>> 25%.  So as is the case with most other conventions, without
>> a tool to help remind us they exist they are unlikely to be
>> followed with enough consistency to be worth putting in place
>> to begin with.
> This kind of response just makes you look combative.  We have a
> convention and we should follow it.  Please do so for your code.

I was responding to myself and adding support to what I suggested
earlier: to add such a warning to GCC to help enforce the convention
on projects that follow it, including GCC.

> 
> If we can cleanly implement this kind of diagnostic, then I would
> certainly suggest we do so, fix our codebase and turn it on by default
> for GCC builds, regardless of its status in -Wall.

I'd be happy to finish and submit the warning when I'm done with
the work whose review prompted me to implement it.

Martin

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Fri, Jun 28, 2019 at 08:55:00AM -0600, Martin Sebor wrote:
>> Jeff reminded me in a code review the other day that GCC does
>> have a guideline for defining POD structs with the keyword
>> "struct" and classes with ctors/dtors using "class":
>> 
>>   https://gcc.gnu.org/codingconventions.html#Struct_Use
>> 
>> I quickly prototyped a warning to see how closely GCC follows
>> this convention.  The result shows that out of just under 800
>> structs and classes defined in GCC sources some 200 use
>> the keyword 'struct' despite having a ctor or dtor, or about
>> 25%.  So as is the case with most other conventions, without
>> a tool to help remind us they exist they are unlikely to be
>> followed with enough consistency to be worth putting in place
>> to begin with.
>
> The goal is not to have the rules adhered to.  The goal is to have
> a more readable / maintainable / etc. codebase.

IMO it's a shame we don't follow that attitude for changelogs,
where apparently the number of spaces between the name and the email
address is of vital importance :-)  Too often a new contributor's
first taste of GCC is a slew of comments about things like that.

But surely it's a valid point that we're not following our own
conventions on the C++ usage.  I miss how consistent the codebase
was in the C days...

That can be fixed by changing the conventions if we no longer think
they're a good idea.  But if someone's willing to change the codebase
to follow (one of) the coventions instead, and willing to add a warning
to keep things that way, then that sounds like a really good thing.
Especially when it's a common convention that others might want a
warning for too.

And I don't think the current state of the struct/class tags is really
a result of making the codebase more maintainable.  I get the feeling
it just kind-of happened.  ISTM we're trying too hard to find a reason
not to clean this up.

(Just replying because I know Martin has been on the receiving end of a
lot of review comments about patches that didn't follow the conventions.
Not necessarily any more than anyone else, and from what I remember
the comments were accurate.  But after all that, I don't think it's fair
to say that the conventions don't really matter.)

Thanks,
Richard

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Segher Boessenkool]

On Fri, Jun 28, 2019 at 07:46:54PM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Fri, Jun 28, 2019 at 08:55:00AM -0600, Martin Sebor wrote:
> >> Jeff reminded me in a code review the other day that GCC does
> >> have a guideline for defining POD structs with the keyword
> >> "struct" and classes with ctors/dtors using "class":
> >> 
> >>   https://gcc.gnu.org/codingconventions.html#Struct_Use
> >> 
> >> I quickly prototyped a warning to see how closely GCC follows
> >> this convention.  The result shows that out of just under 800
> >> structs and classes defined in GCC sources some 200 use
> >> the keyword 'struct' despite having a ctor or dtor, or about
> >> 25%.  So as is the case with most other conventions, without
> >> a tool to help remind us they exist they are unlikely to be
> >> followed with enough consistency to be worth putting in place
> >> to begin with.
> >
> > The goal is not to have the rules adhered to.  The goal is to have
> > a more readable / maintainable / etc. codebase.
> 
> IMO it's a shame we don't follow that attitude for changelogs,
> where apparently the number of spaces between the name and the email
> address is of vital importance :-)  Too often a new contributor's
> first taste of GCC is a slew of comments about things like that.

Yes.  It is important that people make good enough changelogs though.
But when a new contributor gets no comments other than about their
changelog, well, that is less than ideal, right.

> But surely it's a valid point that we're not following our own
> conventions on the C++ usage.  I miss how consistent the codebase
> was in the C days...

I think you misunderstand me.  We *should* follow our coding conventions.
My point was that even if we break the rules in a quarter of cases, we
still win more than we lose, so this doesn't make the rule useless at all.

That isn't saying we shouldn't follow those rules, pretty much the
opposite.


Segher

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Martin Sebor]

On 6/28/19 2:10 PM, Segher Boessenkool wrote:
> On Fri, Jun 28, 2019 at 07:46:54PM +0100, Richard Sandiford wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>>> On Fri, Jun 28, 2019 at 08:55:00AM -0600, Martin Sebor wrote:
>>>> Jeff reminded me in a code review the other day that GCC does
>>>> have a guideline for defining POD structs with the keyword
>>>> "struct" and classes with ctors/dtors using "class":
>>>>
>>>>    https://gcc.gnu.org/codingconventions.html#Struct_Use
>>>>
>>>> I quickly prototyped a warning to see how closely GCC follows
>>>> this convention.  The result shows that out of just under 800
>>>> structs and classes defined in GCC sources some 200 use
>>>> the keyword 'struct' despite having a ctor or dtor, or about
>>>> 25%.  So as is the case with most other conventions, without
>>>> a tool to help remind us they exist they are unlikely to be
>>>> followed with enough consistency to be worth putting in place
>>>> to begin with.
>>>
>>> The goal is not to have the rules adhered to.  The goal is to have
>>> a more readable / maintainable / etc. codebase.
>>
>> IMO it's a shame we don't follow that attitude for changelogs,
>> where apparently the number of spaces between the name and the email
>> address is of vital importance :-)  Too often a new contributor's
>> first taste of GCC is a slew of comments about things like that.
> 
> Yes.  It is important that people make good enough changelogs though.
> But when a new contributor gets no comments other than about their
> changelog, well, that is less than ideal, right.
> 
>> But surely it's a valid point that we're not following our own
>> conventions on the C++ usage.  I miss how consistent the codebase
>> was in the C days...
> 
> I think you misunderstand me.  We *should* follow our coding conventions.
> My point was that even if we break the rules in a quarter of cases, we
> still win more than we lose, so this doesn't make the rule useless at all.

That may be so in some simple cases (perhaps formatting) but it
doesn't follow in all of them, and certainly not in this one.

The rationale for using "struct" for PODs and "class" for classes
with ctors or a dtor is "to signal more information."  That is only
accomplished if this signaling is consistent.  Otherwise, to be
sure, the user still has to confirm whether a struct really is
a POD by carefully inspecting its definition.

Speaking from recent experience, when I need to know whether I'm
dealing with a POD or not I need a reliable answer.  It does no
good to me if I use a struct thinking it's "most likely" a POD
in a template like hash_map or vec that only work correctly with
PODs and corrupts memory otherwise.  Or if I use a struct thinking
copying it is as fast as memcpy when it actually has a ctor that
dynamically allocates memory, only to find out much later after
benchmarking that it slowed down some important algorithm.  So
I would argue that without strict enforcement, this particular
convention is worse than useless: because of its unreliability
it's misleading and dangerous.

Martin

PS We know today that relying on convention for something as
important as this is a bad idea, and we have reliable ways of
enforcing the basic properties of types in containers and
algorithms: type traits and static assertions (they are
available in a limited form even in C++ 98 and C11).  With
those mechanisms in place these risk can be eliminated.  But
then, even with enforcement, the convention ceases to serve
its intended purpose.

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Jeff Law]

On 6/28/19 12:46 PM, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> On Fri, Jun 28, 2019 at 08:55:00AM -0600, Martin Sebor wrote:
>>> Jeff reminded me in a code review the other day that GCC does
>>> have a guideline for defining POD structs with the keyword
>>> "struct" and classes with ctors/dtors using "class":
>>>
>>>   https://gcc.gnu.org/codingconventions.html#Struct_Use
>>>
>>> I quickly prototyped a warning to see how closely GCC follows
>>> this convention.  The result shows that out of just under 800
>>> structs and classes defined in GCC sources some 200 use
>>> the keyword 'struct' despite having a ctor or dtor, or about
>>> 25%.  So as is the case with most other conventions, without
>>> a tool to help remind us they exist they are unlikely to be
>>> followed with enough consistency to be worth putting in place
>>> to begin with.
>>
>> The goal is not to have the rules adhered to.  The goal is to have
>> a more readable / maintainable / etc. codebase.
> 
> IMO it's a shame we don't follow that attitude for changelogs,
> where apparently the number of spaces between the name and the email
> address is of vital importance :-)  Too often a new contributor's
> first taste of GCC is a slew of comments about things like that.
:-)  Given that I regularly deal with the random one off contributors
and cut-n-paste their author information from their email which is
always one space I find the adherence to two spaces in the ChangeLog
annoying.  I still try to fix the, but like all the whitespace stuff,
I'd just assume leave that to commit hooks.  There just isn't a lot of
value in having a human looking for whitespace issues.  I'd much rather
be spending time on more substantial questions.


> 
> But surely it's a valid point that we're not following our own
> conventions on the C++ usage.  I miss how consistent the codebase
> was in the C days...
Sure, it's a valid point and I fully applaud the effort to add warnings
for things where we can and fix this kind of stuff.

> 
> That can be fixed by changing the conventions if we no longer think
> they're a good idea.  But if someone's willing to change the codebase
> to follow (one of) the coventions instead, and willing to add a warning
> to keep things that way, then that sounds like a really good thing.
> Especially when it's a common convention that others might want a
> warning for too.
Likewise -- I'm certainly open to changing conventions where we don't
think they make sense.  But I'm not going to unilaterally do that.


> 
> And I don't think the current state of the struct/class tags is really
> a result of making the codebase more maintainable.  I get the feeling
> it just kind-of happened.  ISTM we're trying too hard to find a reason
> not to clean this up.
I believe the idea was to distinguish between POD and more complex
types.  POD isn't going to do things behind your back, whereas classes
certainly can (and it's often useful, for example RAII constructs).

Jeff

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Richard Sandiford]

Jeff Law <law@redhat.com> writes:
> On 6/28/19 12:46 PM, Richard Sandiford wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>>> On Fri, Jun 28, 2019 at 08:55:00AM -0600, Martin Sebor wrote:
>>>> Jeff reminded me in a code review the other day that GCC does
>>>> have a guideline for defining POD structs with the keyword
>>>> "struct" and classes with ctors/dtors using "class":
>>>>
>>>>   https://gcc.gnu.org/codingconventions.html#Struct_Use
>>>>
>>>> I quickly prototyped a warning to see how closely GCC follows
>>>> this convention.  The result shows that out of just under 800
>>>> structs and classes defined in GCC sources some 200 use
>>>> the keyword 'struct' despite having a ctor or dtor, or about
>>>> 25%.  So as is the case with most other conventions, without
>>>> a tool to help remind us they exist they are unlikely to be
>>>> followed with enough consistency to be worth putting in place
>>>> to begin with.
>>>
>>> The goal is not to have the rules adhered to.  The goal is to have
>>> a more readable / maintainable / etc. codebase.
>> 
>> IMO it's a shame we don't follow that attitude for changelogs,
>> where apparently the number of spaces between the name and the email
>> address is of vital importance :-)  Too often a new contributor's
>> first taste of GCC is a slew of comments about things like that.
> :-)  Given that I regularly deal with the random one off contributors
> and cut-n-paste their author information from their email which is
> always one space I find the adherence to two spaces in the ChangeLog
> annoying.  I still try to fix the, but like all the whitespace stuff,
> I'd just assume leave that to commit hooks.  There just isn't a lot of
> value in having a human looking for whitespace issues.  I'd much rather
> be spending time on more substantial questions.
>
>
>> 
>> But surely it's a valid point that we're not following our own
>> conventions on the C++ usage.  I miss how consistent the codebase
>> was in the C days...
> Sure, it's a valid point and I fully applaud the effort to add warnings
> for things where we can and fix this kind of stuff.
>
>> 
>> That can be fixed by changing the conventions if we no longer think
>> they're a good idea.  But if someone's willing to change the codebase
>> to follow (one of) the coventions instead, and willing to add a warning
>> to keep things that way, then that sounds like a really good thing.
>> Especially when it's a common convention that others might want a
>> warning for too.
> Likewise -- I'm certainly open to changing conventions where we don't
> think they make sense.  But I'm not going to unilaterally do that.
>
>
>> 
>> And I don't think the current state of the struct/class tags is really
>> a result of making the codebase more maintainable.  I get the feeling
>> it just kind-of happened.  ISTM we're trying too hard to find a reason
>> not to clean this up.
> I believe the idea was to distinguish between POD and more complex
> types.  POD isn't going to do things behind your back, whereas classes
> certainly can (and it's often useful, for example RAII constructs).

Yeah.  And that sounds like a reasonable rule if enforced properly
(like Martin says).  I'm certainly not objecting to that. :-)

But what started this thread was that we currently use inconsistent
tags for the same type (struct in one file, class in another).
By definition that can't be following the current rules, since a type
is either POD or not.  And I can't think it was done for readability
or maintainability reasons either.  It looks completely accidental.

What I was objecting to mostly was the pushback against fixing that up
and making the tags self-consistent.  This has come up before, and the
response always seems to be to complain about clang rather than admit
that the current state of things isn't great.

Thanks,
Richard

Re: [PATCH] Fix few build warnings with LLVM toolchain

[Jakub Jelinek]

On Wed, May 29, 2019 at 12:53:44PM +0100, David CARLIER wrote:
> Here a little progress but maybe it s better doing this in small
> "batches" rather than fixing everything in one shot ?

IMHO if we want to do anything about this, we should just in
system.h add
#ifdef __clang__
#pragma clang diagnostic ignored "-Wmismatched-tags"
#endif
perhaps with another guard for some minimum clang version number if the
warning isn't in all clang versions or not all versions support
that pragma.

The warning is really flawed and we shouldn't adjust our source because
of that.

	Jakub

Re: [PATCH] Fix few build warnings with LLVM toolchain

[David CARLIER]

[-- Attachment #1: Type: text/plain, Size: 134 bytes --]

Here a little progress but maybe it s better doing this in small
"batches" rather than fixing everything in one shot ?

Kind regards.

[-- Attachment #2: patch-class2struct-few-fix.diff --]
[-- Type: text/x-patch, Size: 7540 bytes --]

Fixing few build warnings with clang/clang++ of this type:
../.././gcc/coretypes.h:76:1: warning: class 'rtx_def' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
or
../.././gcc/machmode.h:320:1: warning: 'pod_mode' defined as a struct template here but previously declared as a class template; this is valid, but may result in linker errors under the
      Microsoft C++ ABI [-Wmismatched-tags]

The struct/class mismatch is mostly harmless, might be for Microsoft toolchain as mentioned above, but in general for correctness.

Index: gcc/ChangeLog
Index: gcc/cgraph.h
===================================================================
--- gcc/cgraph.h	(revision 271734)
+++ gcc/cgraph.h	(working copy)
@@ -100,11 +100,10 @@ enum symbol_partitioning_class
 
 /* Base of all entries in the symbol table.
    The symtab_node is inherited by cgraph and varpol nodes.  */
-class GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"),
+struct GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"),
 	   chain_next ("%h.next"), chain_prev ("%h.previous")))
   symtab_node
 {
-public:
   friend class symbol_table;
 
   /* Return name.  */
@@ -598,7 +597,6 @@ public:
   /* Section name. Again can be private, if allowed.  */
   section_hash_entry *x_section;
 
-protected:
   /* Dump base fields of symtab nodes to F.  Not to be used directly.  */
   void dump_base (FILE *);
 
@@ -618,7 +616,6 @@ protected:
   bool call_for_symbol_and_aliases_1 (bool (*callback) (symtab_node *, void *),
 				      void *data,
 				      bool include_overwrite);
-private:
   /* Worker for set_section.  */
   static bool set_section (symtab_node *n, void *s);
 
@@ -1505,7 +1502,7 @@ struct cgraph_node_set_def
 typedef cgraph_node_set_def *cgraph_node_set;
 typedef struct varpool_node_set_def *varpool_node_set;
 
-class varpool_node;
+struct varpool_node;
 
 /* A varpool node set is a collection of varpool nodes.  A varpool node
    can appear in multiple sets.  */
@@ -1675,7 +1672,7 @@ struct GTY(()) cgraph_indirect_call_info
 
 struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"),
 	    for_user)) cgraph_edge {
-  friend class cgraph_node;
+  friend struct cgraph_node;
   friend class symbol_table;
 
   /* Remove the edge in the cgraph.  */
@@ -1856,8 +1853,7 @@ private:
 /* The varpool data structure.
    Each static variable decl has assigned varpool_node.  */
 
-class GTY((tag ("SYMTAB_VARIABLE"))) varpool_node : public symtab_node {
-public:
+struct GTY((tag ("SYMTAB_VARIABLE"))) varpool_node : public symtab_node {
   /* Dump given varpool node to F.  */
   void dump (FILE *f);
 
@@ -1975,7 +1971,6 @@ public:
      if we did not do any inter-procedural code movement.  */
   unsigned used_by_single_function : 1;
 
-private:
   /* Assemble thunks and aliases associated to varpool node.  */
   void assemble_aliases (void);
 
@@ -2074,9 +2069,9 @@ struct asmname_hasher : ggc_ptr_hash <sy
 class GTY((tag ("SYMTAB"))) symbol_table
 {
 public:
-  friend class symtab_node;
-  friend class cgraph_node;
-  friend class cgraph_edge;
+  friend struct symtab_node;
+  friend struct cgraph_node;
+  friend struct cgraph_edge;
 
   symbol_table (): cgraph_max_uid (1), cgraph_max_summary_id (0),
   edges_max_uid (1), edges_max_summary_id (0)
Index: gcc/coretypes.h
===================================================================
--- gcc/coretypes.h	(revision 271734)
+++ gcc/coretypes.h	(working copy)
@@ -65,7 +65,7 @@ template<typename> class opt_mode;
 typedef opt_mode<scalar_mode> opt_scalar_mode;
 typedef opt_mode<scalar_int_mode> opt_scalar_int_mode;
 typedef opt_mode<scalar_float_mode> opt_scalar_float_mode;
-template<typename> class pod_mode;
+template<typename> struct pod_mode;
 typedef pod_mode<scalar_mode> scalar_mode_pod;
 typedef pod_mode<scalar_int_mode> scalar_int_mode_pod;
 typedef pod_mode<fixed_size_mode> fixed_size_mode_pod;
@@ -73,7 +73,7 @@ typedef pod_mode<fixed_size_mode> fixed_
 /* Subclasses of rtx_def, using indentation to show the class
    hierarchy, along with the relevant invariant.
    Where possible, keep this list in the same order as in rtl.def.  */
-class rtx_def;
+struct rtx_def;
   class rtx_expr_list;           /* GET_CODE (X) == EXPR_LIST */
   class rtx_insn_list;           /* GET_CODE (X) == INSN_LIST */
   class rtx_sequence;            /* GET_CODE (X) == SEQUENCE */
@@ -138,9 +138,9 @@ struct gomp_teams;
 /* Subclasses of symtab_node, using indentation to show the class
    hierarchy.  */
 
-class symtab_node;
+struct symtab_node;
   struct cgraph_node;
-  class varpool_node;
+  struct varpool_node;
 
 union section;
 typedef union section section;
Index: gcc/dumpfile.h
===================================================================
--- gcc/dumpfile.h	(revision 271734)
+++ gcc/dumpfile.h	(working copy)
@@ -647,7 +647,7 @@ extern void dump_combine_total_stats (FI
 /* In cfghooks.c  */
 extern void dump_bb (FILE *, basic_block, int, dump_flags_t);
 
-struct opt_pass;
+class opt_pass;
 
 namespace gcc {
 
Index: gcc/hash-table.h
===================================================================
--- gcc/hash-table.h	(revision 271734)
+++ gcc/hash-table.h	(working copy)
@@ -347,7 +347,7 @@ hash_table_mod2 (hashval_t hash, unsigne
   return 1 + mul_mod (hash, p->prime - 2, p->inv_m2, p->shift);
 }
 
-class mem_usage;
+struct mem_usage;
 
 /* User-facing hash table type.
 
Index: gcc/ipa-prop.h
===================================================================
--- gcc/ipa-prop.h	(revision 271734)
+++ gcc/ipa-prop.h	(working copy)
@@ -182,7 +182,7 @@ struct GTY (()) ipa_jump_func
   /* Information about value range, containing valid data only when vr_known is
      true.  The pointed to structure is shared betweed different jump
      functions.  Use ipa_set_jfunc_vr to set this field.  */
-  struct value_range_base *m_vr;
+  class value_range_base *m_vr;
 
   enum jump_func_type type;
   /* Represents a value of a jump function.  pass_through is used only in jump
@@ -589,7 +589,7 @@ class GTY((for_user)) ipa_edge_args
 /* Return the number of actual arguments. */
 
 static inline int
-ipa_get_cs_argument_count (struct ipa_edge_args *args)
+ipa_get_cs_argument_count (class ipa_edge_args *args)
 {
   return vec_safe_length (args->jump_functions);
 }
Index: gcc/ipa-ref.h
===================================================================
--- gcc/ipa-ref.h	(revision 271734)
+++ gcc/ipa-ref.h	(working copy)
@@ -22,8 +22,8 @@ along with GCC; see the file COPYING3.
 #define GCC_IPA_REF_H
 
 struct cgraph_node;
-class varpool_node;
-class symtab_node;
+struct varpool_node;
+struct symtab_node;
 
 
 /* How the reference is done.  */
Index: gcc/lto-streamer.h
===================================================================
--- gcc/lto-streamer.h	(revision 271734)
+++ gcc/lto-streamer.h	(working copy)
@@ -360,9 +360,8 @@ private:
 };
 
 /* Structure used as buffer for reading an LTO file.  */
-class lto_input_block
+struct lto_input_block
 {
-public:
   /* Special constructor for the string table, it abuses this to
      do random access but use the uhwi decoder.  */
   lto_input_block (const char *data_, unsigned int p_, unsigned int len_,
Index: gcc/tree-pass.h
===================================================================
--- gcc/tree-pass.h	(revision 271734)
+++ gcc/tree-pass.h	(working copy)
@@ -132,7 +132,7 @@ protected:
   }
 };
 
-class varpool_node;
+struct varpool_node;
 struct cgraph_node;
 struct lto_symtab_encoder_d;