Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some const qualifiers #16856

Closed
p5pRT opened this issue Mar 1, 2019 · 12 comments
Closed

Add some const qualifiers #16856

p5pRT opened this issue Mar 1, 2019 · 12 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Mar 1, 2019

Migrated from rt.perl.org#133874 (status was 'open')

Searchable as RT133874$

@p5pRT
Copy link
Author

p5pRT commented Mar 1, 2019

From peter@eisentraut.org

This avoids compiler warnings if something including these header
files (via perl.h) is compiled using -Wcast-qual.

@p5pRT
Copy link
Author

p5pRT commented Mar 1, 2019

From peter@eisentraut.org

0001-Add-some-const-qualifiers.patch
From 06cec305baa2e694f98bdf98bf408ca90476196d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 1 Mar 2019 15:52:54 +0100
Subject: [PATCH] Add some const qualifiers

This avoids compiler warnings if something including these header
files (via perl.h) is compiled using -Wcast-qual.
---
 hv_func.h       | 10 +++++-----
 inline.h        |  4 ++--
 sbox32_hash.h   |  4 ++--
 stadtx_hash.h   |  2 +-
 zaphod32_hash.h |  2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hv_func.h b/hv_func.h
index 111de93f1e..d6b8450f85 100644
--- a/hv_func.h
+++ b/hv_func.h
@@ -39,26 +39,26 @@
 # define __PERL_HASH_SEED_BYTES 16
 # define __PERL_HASH_STATE_BYTES 32
 # define __PERL_HASH_SEED_STATE(seed,state) S_perl_siphash_seed_state(seed,state)
-# define __PERL_HASH_WITH_STATE(state,str,len) S_perl_hash_siphash_2_4_with_state((state),(U8*)(str),(len))
+# define __PERL_HASH_WITH_STATE(state,str,len) S_perl_hash_siphash_2_4_with_state((state),(const U8*)(str),(len))
 #elif defined(PERL_HASH_FUNC_SIPHASH13)
 # define __PERL_HASH_FUNC "SIPHASH_1_3"
 # define __PERL_HASH_SEED_BYTES 16
 # define __PERL_HASH_STATE_BYTES 32
 # define __PERL_HASH_SEED_STATE(seed,state) S_perl_siphash_seed_state(seed,state)
-# define __PERL_HASH_WITH_STATE(state,str,len) S_perl_hash_siphash_1_3_with_state((state),(U8*)(str),(len))
+# define __PERL_HASH_WITH_STATE(state,str,len) S_perl_hash_siphash_1_3_with_state((state),(const U8*)(str),(len))
 #elif defined(PERL_HASH_FUNC_STADTX)
 # define __PERL_HASH_FUNC "STATDX"
 # define __PERL_HASH_SEED_BYTES 16
 # define __PERL_HASH_STATE_BYTES 32
 # define __PERL_HASH_SEED_STATE(seed,state) stadtx_seed_state(seed,state)
-# define __PERL_HASH_WITH_STATE(state,str,len) (U32)stadtx_hash_with_state((state),(U8*)(str),(len))
+# define __PERL_HASH_WITH_STATE(state,str,len) (U32)stadtx_hash_with_state((state),(const U8*)(str),(len))
 # include "stadtx_hash.h"
 #elif defined(PERL_HASH_FUNC_ZAPHOD32)
 # define __PERL_HASH_FUNC "ZAPHOD32"
 # define __PERL_HASH_SEED_BYTES 12
 # define __PERL_HASH_STATE_BYTES 12
 # define __PERL_HASH_SEED_STATE(seed,state) zaphod32_seed_state(seed,state)
-# define __PERL_HASH_WITH_STATE(state,str,len) (U32)zaphod32_hash_with_state((state),(U8*)(str),(len))
+# define __PERL_HASH_WITH_STATE(state,str,len) (U32)zaphod32_hash_with_state((state),(const U8*)(str),(len))
 # include "zaphod32_hash.h"
 #endif
 
@@ -95,7 +95,7 @@
 
 #define _PERL_HASH_WITH_STATE(state,str,len)                                            \
     (LIKELY(len <= SBOX32_MAX_LEN)                                                      \
-        ? sbox32_hash_with_state((state + __PERL_HASH_STATE_BYTES),(U8*)(str),(len))    \
+        ? sbox32_hash_with_state((state + __PERL_HASH_STATE_BYTES),(const U8*)(str),(len))    \
         : __PERL_HASH_WITH_STATE((state),(str),(len)))
 
 #endif
diff --git a/inline.h b/inline.h
index 0a3c47ab39..7cae6bdd9b 100644
--- a/inline.h
+++ b/inline.h
@@ -455,7 +455,7 @@ S_is_utf8_invariant_string_loc(const U8* const s, STRLEN len, const U8 ** ep)
         /* Here, we know we have at least one full word to process.  Process
          * per-word as long as we have at least a full word left */
         do {
-            if ((* (PERL_UINTMAX_T *) x) & PERL_VARIANTS_WORD_MASK)  {
+            if ((* (const PERL_UINTMAX_T *) x) & PERL_VARIANTS_WORD_MASK)  {
 
                 /* Found a variant.  Just return if caller doesn't want its
                  * exact position */
@@ -466,7 +466,7 @@ S_is_utf8_invariant_string_loc(const U8* const s, STRLEN len, const U8 ** ep)
 #  if   BYTEORDER == 0x1234 || BYTEORDER == 0x12345678    \
      || BYTEORDER == 0x4321 || BYTEORDER == 0x87654321
 
-                *ep = x + _variant_byte_number(* (PERL_UINTMAX_T *) x);
+                *ep = x + _variant_byte_number(* (const PERL_UINTMAX_T *) x);
                 assert(*ep >= s && *ep < send);
 
                 return FALSE;
diff --git a/sbox32_hash.h b/sbox32_hash.h
index 766102f32a..32b1158499 100644
--- a/sbox32_hash.h
+++ b/sbox32_hash.h
@@ -1460,7 +1460,7 @@ SBOX32_STATIC_INLINE void sbox32_seed_state128 (
     const U8 *seed_ch,
     U8 *state_ch
 ) {
-    U32 *seed= (U32 *)seed_ch;
+    const U32 *seed= (const U32 *)seed_ch;
     U32 *state= (U32 *)state_ch;
     U32 *state_cursor = state + 1;
     U32 *sbox32_end = state + 1 + (256 * SBOX32_MAX_LEN);
@@ -1495,7 +1495,7 @@ SBOX32_STATIC_INLINE U32 sbox32_hash_with_state(
     const U8 *key,
     const STRLEN key_len
 ) {
-    U32 *state= (U32 *)state_ch;
+    const U32 *state= (const U32 *)state_ch;
     U32 hash = *state;
     switch (key_len) {
         default: return zaphod32_hash_with_state(state_ch, key, key_len);
diff --git a/stadtx_hash.h b/stadtx_hash.h
index bd09c2f938..6582303fdc 100644
--- a/stadtx_hash.h
+++ b/stadtx_hash.h
@@ -187,7 +187,7 @@ STADTX_STATIC_INLINE U64 stadtx_hash_with_state(
     const U8 *key,
     const STRLEN key_len
 ) {
-    U64 *state= (U64 *)state_ch;
+    const U64 *state= (const U64 *)state_ch;
     STRLEN len = key_len;
     U64 v0= state[0] ^ ((key_len+1) * STADTX_K0_U64);
     U64 v1= state[1] ^ ((key_len+2) * STADTX_K1_U64);
diff --git a/zaphod32_hash.h b/zaphod32_hash.h
index c9b60ccb32..3db57dd932 100644
--- a/zaphod32_hash.h
+++ b/zaphod32_hash.h
@@ -213,7 +213,7 @@ U32 zaphod32_hash_with_state(
     const U8 *key,
     const STRLEN key_len
 ) {
-    U32 *state= (U32 *)state_ch;
+    const U32 *state= (const U32 *)state_ch;
     const U8 *end;
     STRLEN len = key_len;
     U32 v0= state[0];
-- 
2.20.1

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2019

From @jkeenan

On Fri, 01 Mar 2019 20​:00​:33 GMT, peter@​eisentraut.org wrote​:

This avoids compiler warnings if something including these header
files (via perl.h) is compiled using -Wcast-qual.

We can acknowledge that if one requests this type of build-time warning, one gets over 11,000 instances thereof​:

#####
$ sh ./Configure -des -Dusedevel -Dcc="gcc -Wcast-qual"

# before patch (de59f38)
$ report-build-warnings make.cast-qual.output.txt
File​: make.cast-qual.output.txt

  Wcast-qual 11777
  Wimplicit-fallthrough= 35
  Wunused-variable 2
#####

However, even after applying your patch we still end up with over 10,000 instances of that warning.

#####
# after patch
$ report-build-warnings make.cast-qual.branch.output.txt
File​: make.cast-qual.branch.output.txt

  Wcast-qual 10873
  Wimplicit-fallthrough= 35
  Wunused-variable 2
#####

So the benefit of the patch is marginal, and the benefit has to be weighed against the cost of side effects resulting from changes so deep in the source code.

In seven years of following this list, I have never seen a report of someone compiling with '-Wcast-qual'. We *do* try to eliminate build-time warnings that arise when people build with the most common kinds of ./Configure options. But I myself don't see the point of patching the code to avoid only a small percentage of the warnings generated when someone builds with a compiler option for which we have no demonstrated constituency.

Other people's opinions?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2019

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2019

From @tonycoz

On Sat, Mar 02, 2019 at 04​:50​:05PM -0800, James E Keenan via RT wrote​:

On Fri, 01 Mar 2019 20​:00​:33 GMT, peter@​eisentraut.org wrote​:

This avoids compiler warnings if something including these header
files (via perl.h) is compiled using -Wcast-qual.

We can acknowledge that if one requests this type of build-time warning, one gets over 11,000 instances thereof​:

#####
$ sh ./Configure -des -Dusedevel -Dcc="gcc -Wcast-qual"

# before patch (de59f38)
$ report-build-warnings make.cast-qual.output.txt
File​: make.cast-qual.output.txt

Wcast-qual 11777
Wimplicit-fallthrough= 35
Wunused-variable 2
#####

However, even after applying your patch we still end up with over 10,000 instances of that warning.

#####
# after patch
$ report-build-warnings make.cast-qual.branch.output.txt
File​: make.cast-qual.branch.output.txt

Wcast-qual 10873
Wimplicit-fallthrough= 35
Wunused-variable 2
#####

So the benefit of the patch is marginal, and the benefit has to be weighed against the cost of side effects resulting from changes so deep in the source code.

In seven years of following this list, I have never seen a report of someone compiling with '-Wcast-qual'. We *do* try to eliminate build-time warnings that arise when people build with the most common kinds of ./Configure options. But I myself don't see the point of patching the code to avoid only a small percentage of the warnings generated when someone builds with a compiler option for which we have no demonstrated constituency.

Other people's opinions?

If the changes correctly fix casts that convert const pointers to
non-const pointers without breaking the API, then it's an improvement,
since such casts can be dangerous.

I haven't looked closely enough at the patch to check that though.

Tony

@p5pRT
Copy link
Author

p5pRT commented Mar 5, 2019

From peter@eisentraut.org

On 2019-03-03 01​:50, James E Keenan via RT wrote​:

We can acknowledge that if one requests this type of build-time warning, one gets over 11,000 instances thereof​:

However, even after applying your patch we still end up with over 10,000 instances of that warning.

The purpose of the patch is not to reduce those warnings in a Perl core
build but to reduce those warnings for third-party packages that include
Perl header files. Since these are inline functions, any warnings they
cause will affect any users of those header files.

@jkeenan
Copy link
Contributor

jkeenan commented Jul 10, 2022

@tonycoz, this patch proposal hasn't picked up any support since March 2019. I believe there's no call for it. Can we close this ticket?

@jkeenan jkeenan added the Closable? We might be able to close this ticket, but we need to check with the reporter label Jul 10, 2022
@tonycoz
Copy link
Contributor

tonycoz commented Jul 12, 2022

I'll work up and up to date patch.

While we don't build with -Wcast-qual we do have the MUTABLE_PTR() family of macros are specifically intended to avoid casting away const.

There's also some existing GCC_DIAG_IGNORE(-Wcast-qual) in the code.

@jkeenan
Copy link
Contributor

jkeenan commented Sep 26, 2022

I'll work up and up to date patch.

While we don't build with -Wcast-qual we do have the MUTABLE_PTR() family of macros are specifically intended to avoid casting away const.

There's also some existing GCC_DIAG_IGNORE(-Wcast-qual) in the code.

@tonycoz have you been able to make any headway on this ticket?

@tonycoz
Copy link
Contributor

tonycoz commented Sep 26, 2022

I'll look at this again today.

@jkeenan
Copy link
Contributor

jkeenan commented Oct 26, 2023

I'll look at this again today.

@tonycoz can you give us an update on the status of this ticket? Thanks.

@tonycoz
Copy link
Contributor

tonycoz commented Oct 26, 2023

The original patch was merged, I'll leave this on my own stack of quality fixes.

@tonycoz tonycoz closed this as completed Oct 26, 2023
@jkeenan jkeenan removed the Closable? We might be able to close this ticket, but we need to check with the reporter label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants