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] Fix few build warnings with LLVM toolchain


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


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