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

Decast nxt_cpymem() #1489

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Decast nxt_cpymem() #1489

merged 1 commit into from
Nov 19, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Nov 13, 2024

    Decast nxt_cpymem()
    
    nxt_cpymem() is basically mempcpy(3)
    
    Like mempcpy() nxt_cpymem() returns a void *.
    
    nxt_cpymem() is implemented as a wrapper around memcpy(3), however
    before returning the new pointer value we cast the return of memcpy(3)
    to a u_char *, then add the length parameter to it.
    
    I guess this was done to support compilers that do not support
    arithmetic on void pointers as the C standard forbids it.
    
    However since we removed support for compilers other than GCC and Clang
    (ending in commit 9cd11133 ("Remove support for Sun's Sun Studio/SunPro
    C compiler")) this is no longer an issue as both GCC and Clang support
    arithmetic on void pointers (without the -pedantic option).
    
    While removing the unnecessary casting in this case doesn't necessarily
    improve type-safety (as we're dealing with void *'s in and out), it does
    just make the code that little more readable.
    
    Oh and for interest we have actually already been relying on this
    extension
    
      src/nxt_array.c:143:40: warning: arithmetic on a pointer to void is a GNU extension [-Wgnu-pointer-arith]
        143 |             nxt_memcpy(data, src->elts + (i * size), size);
            |                              ~~~~~~~~~ ^
      src/nxt_string.h:45:24: note: expanded from macro 'nxt_memcpy'
         45 |     (void) memcpy(dst, src, length)
            |                        ^~~
    
    which was introduced in e2b53e16 ("Added "rootfs" feature.") back in
    2020.
    
    Link: <https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html>
    Link: <https://clang.llvm.org/docs/LanguageExtensions.html#introduction>
    Signed-off-by: Andrew Clayton <[email protected]>

@ac000 ac000 marked this pull request as ready for review November 13, 2024 16:36
@callahad
Copy link
Collaborator

@hongzhidao @avahahn Would either of you like to review?

@hongzhidao
Copy link
Contributor

Hi,
I have no strong preference either way. If the testing results look good, feel free to do it.
Thanks.

nxt_cpymem() is basically mempcpy(3)

Like mempcpy() nxt_cpymem() returns a void *.

nxt_cpymem() is implemented as a wrapper around memcpy(3), however
before returning the new pointer value we cast the return of memcpy(3)
to a u_char *, then add the length parameter to it.

I guess this was done to support compilers that do not support
arithmetic on void pointers as the C standard forbids it.

However since we removed support for compilers other than GCC and Clang
(ending in commit 9cd1113 ("Remove support for Sun's Sun Studio/SunPro
C compiler")) this is no longer an issue as both GCC and Clang support
arithmetic on void pointers (without the -pedantic option) by treating
the size of a void as 1.

While removing the unnecessary casting in this case doesn't necessarily
improve type-safety (as we're dealing with void *'s in and out), it does
just make the code that little more readable.

Oh and for interest we have actually already been relying on this
extension

  src/nxt_array.c:143:40: warning: arithmetic on a pointer to void is a GNU extension [-Wgnu-pointer-arith]
    143 |             nxt_memcpy(data, src->elts + (i * size), size);
        |                              ~~~~~~~~~ ^
  src/nxt_string.h:45:24: note: expanded from macro 'nxt_memcpy'
     45 |     (void) memcpy(dst, src, length)
        |                        ^~~

which was introduced in e2b53e1 ("Added "rootfs" feature.") back in
2020.

Link: <https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html>
Link: <https://clang.llvm.org/docs/LanguageExtensions.html#introduction>
Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented Nov 19, 2024

Note in the commit message that GCC et al treat sizeof(void) as 1.

$ git range-diff 2bcfd5dc...2fe744cd
1:  2bcfd5dc ! 1:  2fe744cd Decast nxt_cpymem()
    @@ Commit message
         However since we removed support for compilers other than GCC and Clang
         (ending in commit 9cd11133 ("Remove support for Sun's Sun Studio/SunPro
         C compiler")) this is no longer an issue as both GCC and Clang support
    -    arithmetic on void pointers (without the -pedantic option).
    +    arithmetic on void pointers (without the -pedantic option) by treating
    +    the size of a void as 1.
     
         While removing the unnecessary casting in this case doesn't necessarily
         improve type-safety (as we're dealing with void *'s in and out), it does

@ac000
Copy link
Member Author

ac000 commented Nov 19, 2024

Rebased with master

$ git range-diff 2fe744cd...4f041328
-:  -------- > 1:  2d4624f0 Make nxt_tstr_is_js() macro public in header
-:  -------- > 2:  a92d8149 http: Refactor format field in nxt_router_access_log_conf_t
-:  -------- > 3:  a760e24a http: Introduce nxt_router_access_log_format_t structure
-:  -------- > 4:  bc838c5e http: Support JSON format in access log
-:  -------- > 5:  a3551790 tests: Add tests for JSON format access log
1:  2fe744cd = 6:  4f041328 Decast nxt_cpymem()

@ac000 ac000 merged commit 4f04132 into nginx:master Nov 19, 2024
25 checks passed
@ac000 ac000 deleted the decast branch November 19, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants