Skip to content

Conversation

@dpelle
Copy link
Member

@dpelle dpelle commented Sep 23, 2023

Change fixes this kind of compilation warnings with clang-17:

proto/if_python3.pro:13:20: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
   13 | int python3_version();
      |                    ^
      |                     void

@pheiduck
Copy link
Contributor

Interesting how do you compile vim?
I have made a PR to get clang-17 on the CI but it runs fine on latest patch.:
#12745

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #13166 (a410778) into master (ceee7a8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head a410778 differs from pull request most recent head 664fde4. Consider uploading reports for the commit 664fde4 to get more accurate results

@@            Coverage Diff             @@
##           master   #13166      +/-   ##
==========================================
- Coverage   82.13%   82.12%   -0.01%     
==========================================
  Files         160      160              
  Lines      195308   195308              
  Branches    43823    43823              
==========================================
- Hits       160412   160395      -17     
- Misses      22058    22075      +17     
  Partials    12838    12838              
Flag Coverage Δ
huge-clang-Array 82.73% <100.00%> (-0.01%) ⬇️
linux 82.73% <100.00%> (-0.01%) ⬇️
mingw-x64-HUGE 76.71% <100.00%> (+<0.01%) ⬆️
mingw-x86-HUGE 77.22% <100.00%> (+<0.01%) ⬆️
windows 78.30% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/os_unix.c 66.97% <ø> (-0.05%) ⬇️
src/debugger.c 89.17% <100.00%> (ø)
src/if_python3.c 74.45% <100.00%> (ø)

... and 12 files with indirect coverage changes

@dpelle
Copy link
Member Author

dpelle commented Sep 24, 2023

@pheiduck asked:

Interesting how do you compile vim?
I have made a PR to get clang-17 on the CI but it runs fine on latest patch.:
#12745

To reproduce the warning with clang-17, I had uncommented out this line to enable -Wall -Wmissing-prototypes:

$ git diff
diff --git a/src/Makefile b/src/Makefile
index 4d0a5ebad..a5c0d9dea 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -602,7 +602,7 @@ CClink = $(CC)
 #CFLAGS = -g -O2 -fno-strength-reduce -Wall -Wmissing-prototypes
 #CFLAGS = -g -Wall -Wmissing-prototypes
 #CFLAGS = -O6 -fno-strength-reduce -Wall -Wshadow -Wmissing-prototypes
-#CFLAGS = -g -DDEBUG -Wall -Wshadow -Wmissing-prototypes
+CFLAGS = -g -DDEBUG -Wall -Wshadow -Wmissing-prototypes
 #CFLAGS = -g -O2 '-DSTARTUPTIME="vimstartup"' -fno-strength-reduce -Wall -Wmissing-prototypes

 # Use this with GCC to check for mistakes, unused arguments, etc.

I now see that the warning is not specific to clang-17 actually, it also happens with clang-14, clang-15, gcc-9, gcc-11 for example, so I'll reword the commit message.

Change fixes this kind of compilation warnings with clang:
```
proto/if_python3.pro:13:20: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
   13 | int python3_version();
      |                    ^
      |                     void
```
@dpelle dpelle force-pushed the dpelle/fix-clang-17-warnings branch from a410778 to 664fde4 Compare September 24, 2023 07:38
@dpelle dpelle changed the title fix: clang-17 compilation warnings fix: clang compilation warnings with -Wstrict-prototypes Sep 24, 2023
@chrisbra chrisbra closed this in 4927bc7 Sep 24, 2023
@ychin
Copy link
Contributor

ychin commented Sep 25, 2023

I think if certain warnings are important, such as -Wmissing-prototypes and -Wstrict-prototypes, we should turn them on in ci/config.mk.sed or ci/config.mk.gcc.sed or ci/config.mk.clang.sed. This way it will actually be checked in CI.

Otherwise it's just whack-a-mole every time. We should either decide if such warnings are important (in which case we check them in CI), or unimportant (in which case we don't bother fixing them).

@ychin
Copy link
Contributor

ychin commented Sep 26, 2023

Hmm, I tried adding this to CI and seems like GTK and Ruby headers don't really like them, so it would be a little annoying to just add them in unless we also add warning suppression when including those headers…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants