public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Gerald Pfeifer <gerald@pfeifer.com>
To: Florian Weimer <fweimer@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Notes on the warnings-as-errors change in GCC 14
Date: Sat, 10 Feb 2024 12:00:34 +0100 (CET)	[thread overview]
Message-ID: <2058c7ba-20af-7cb5-f15d-ec0653f76903@pfeifer.com> (raw)
In-Reply-To: <87v876ssxg.fsf@oldenburg.str.redhat.com>

Hi Florian,

that's been quite a bit. Thank you for putting this together so 
comprehensively and thoughtfully, with examples and background!

Note many of my points are suggestions or questions, especially if phrased 
as questions or using maybe or similar, so for your consideration.

On Fri, 2 Feb 2024, Florian Weimer wrote:
>  htdocs/gcc-14/porting_to.html | 465 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 465 insertions(+)
> +
> +<h4 id="int-conversion">Using pointers as integers and vice versa (<code>-Werror=int-conversion</code>)</h4>

> +It makes sense to address missing <code>int</code> type, implicit

Should this be plural here ("<code>int</code> types") or some adding a 
word such as "declaration"? Genuine question.

> +Some of them will be caused by missing types resulting
> +in <code>int</code>, and the default <code>int</code> return type of
> +implicitly declared functions.

...resulting in implicit <code>int</code>... or something like that?
Not sure how to be phrase it.

> +<p> Note that in some cases, it may be more appropriate to pass the
> +address of an integer variable instead of a cast of the variable's
> +value.

I'd omit the comma here since I don't feel the added emphasis is 
necessary.

(And maybe simply say "In some cases it may..."?)

> +GCC no longer casts all pointer types to all other pointer types.

Do you mean it no longer does so implicitly, or not at all? That is,
there are now cases where even an explicit cast such as

  foo_p = (foo_type*) bar_p

no longer works? Or just

  foo_p = bar_p

no longer works for all combinations?

> +To fix the compilation errors resulting from that, you can add the

"To fix compilation errors..."

> +appropriate casts, and maybe consider using code <code>void *</code>
> +in more places (particularly for old programs that predate the
> +introduction of <code>void</code> into the C language).

Here I got confused.

At first I thought I was reading that <code>void *</void> should be used 
for cases where <code>void</void> did not exist yet. Now I think I 
understand: this is about programs where <code>void *</code> was not used 
since it was not part of the language yet - and the encouragement is to 
update such old code by using it. 

If so, how about making the second case <code>void *</code>, too?

> +Programs that do not carefully track pointer types are likely to
> +contain aliasing violations, so consider building
> +with <code>-fno-strict-aliasing</code> as well.

I suggest omitting "as well" here.

> (Whether the casts
> +are written manually or performed by GCC automatically does not make a
> +difference in terms of strict aliasing violations.)

Here I'd just say "Whether casts are", omitting "the".

> +#include &lt;stddef.h>

I *think* we may need to use &gt; here instead of plain '>', though I may 
be wrong.


> +To correct this, the callback function should be defined with the
> +correct type

"To address this ... be defined with the correct type"

(To avoid the double "correct"ion.)

> +<pre>
> +int
> +compare (<ins>const void *a1</ins>, <ins>const void *b1</ins>)
> +{
> +  <ins>char *const *a = a1;</ins>
> +  <ins>char *const *b = b1;</ins>
> +  return strcmp (*a, *b);
> +}
> +</pre>

Great that you include this example here, that really helps!

Just why "const void *a1" versus "char *const *a", that is, the different 
placement of const?

And should it be "const void **a1" and similarly for "b1"??

> +As mentioned initially, adding the cast here would not eliminate any
> +strict aliasing violation in the implementation of
> +the <code>operate</code> function.

Really great you are pointing this out explicitly; thank you!

> +It may be possible to avoid writing manual casts with
> +the <code>-fplan9-extensions</code> options and unnamed

...option... (singular)

> +unrelated to the actual objective of the probe.  These failed probes
> +tend to consistently disable program features and their tests, which
> +means that an unexpected probe failure may result in silently dropping
> +features.

Omit "consistently"? I'm not sure what it adds here. And simplify the 
second half, something like

  These failed probes tend to disable program features (and their tests), 
  resulting in silently dropping features.

> +In cases where this is a concern, generated <code>config.log</code>,
> +<code>config.h</code> and other source code files can be compared
> +using <a href="https://www.gnu.org/software/diffutils/">diff</a>,
> +to ensure there are no unexpected differences.

I wouldn't link to GNU diffutils here; just refer to the <code>diff</code> 
command - or even omit that aspect and leave it at "can be compared".

> +This phenomenon also impacts similar procedures part of CMake, Meson,
> +and various build tools for C extension modules of scripting
> +languages.

"...procedures in CMake..."?

> +<p>
> +Autoconf has supported C99 compilers since at least version 2.69 in
> +its generic, core probes.  (Specific functionality probes may have
> +issues addressed in more recent versions.)  Versions before 2.69 may
> +have generic probes (for example for standard C support) that rely on
> +C features that were removed in 1999 and thus fail with GCC 14.

"...removed in C99..."? As opposed to compilers removing them in 1999?

> +Sources that cannot be ported to standard C can be compiled
> +with <code>-fpermissive</code>, <code>-std=gnu89</code>,
> +or <code>-std=c89</code>.  Despite their names, the latter two options
> +turn on support for pre-standard C constructs, too.  With the
> +<code>-fpermissive</code> options

"option" (singular)

> +Some build systems do not pass the <code>CFLAGS</code> environment
> +or <code>make</code> variable to all parts of the builds

Is "make" a common variable? What is the context here?

> +Not listed here are <code>-Wimplicit-int</code>
> +and <code>-Wdeclaration-missing-parameter-type</code> because they
> +should be straightforward to address in a code generator.

More personal style, I'd say "Not included here..." which makes it more 
specific to the script as opposed to the overall section. To me at least. 
:-)

> +This section concerns potential future changes related to language
> +features from the C standard and other backwards compatibility
> +hazards.  These plans may change and are mentioned here only to give
> +guidance which source-level changes to prioritize for future compiler
> +compatibility.

"compiler compatibility" or "standards compatibility"? Maybe just 
"compatibility"?

> +<p>
> +It is unclear at which point GCC can enable the C23 <code>bool</code>
> +keyword by default (making the <code>bool</code> type available
> +without including <code>#include &lt;stdbool.h></code> explicitly).

Does C every include some header files implicitly? If not, I'd omit
"explicitly" above.

> +Many programs define their own <code>bool</code> types, sometimes with
> +a different size of the already-available <code>_Bool</code> type.

"different size than"

And "already available" or maybe "built-in"?


For the benefit of the doubt: Okay, and thank you, modulo feedback from 
Jonathan and my two responses.

Gerald

  parent reply	other threads:[~2024-02-10 11:00 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-02 16:59 Florian Weimer
2024-02-02 18:08 ` Jonathan Wakely
2024-02-15 15:57   ` Florian Weimer
2024-02-02 19:06 ` Jonathan Wakely
2024-02-08  7:04 ` Sam James
2024-02-15 16:07   ` Florian Weimer
2024-02-15 16:20     ` Sam James
2024-02-09 23:07 ` Gerald Pfeifer
2024-02-15 16:22   ` Florian Weimer
2024-02-15 22:58     ` Gerald Pfeifer
2024-02-15 16:49   ` Florian Weimer
2024-02-10 11:00 ` Gerald Pfeifer [this message]
2024-02-15 16:44   ` Florian Weimer
2024-02-16 21:23     ` Gerald Pfeifer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2058c7ba-20af-7cb5-f15d-ec0653f76903@pfeifer.com \
    --to=gerald@pfeifer.com \
    --cc=fweimer@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).