Turn on useful warnings from gcc #218

kalvdans · 2018-02-17T20:04:50Z

Fix code that gcc warns for. Remove unused variables, const poisoning,
and use declare functions (void) instead of ().

Please note that it changes the signatures of the API. The ABI is not changed.

I need someone to double-check the CMakeLists.txt changes.

Fix code that gcc warns for. Remove unused variables, const poisoning, and use declare functions (void) instead of ().

Function definitions in header files makes some functions unused in some compilation units.

kalvdans · 2018-02-18T21:14:56Z

@FrancescAlted, what is your opinion on annotating strings that should not be modified with 'const' in the API?

Use add_compile_options() instead of fiddling with variables. The options will apply to both C and C++ so I had to drop -Wstrict-prototypes.

I better not touch Windows flags since I can't easily test out the changes locally.

kalvdans · 2018-02-19T14:45:49Z

Ok, this pull request is ready for review. @estan, @FrancescAlted. We also need to agree on the timing to introduce these API changes and what to write in the release notes.

FrancescAlted · 2018-02-19T15:48:19Z

blosc/blosclz.c

@@ -79,7 +79,7 @@



-static inline uint8_t *get_run(uint8_t *ip, const uint8_t *ip_bound, const uint8_t *ref) {
+static uint8_t *get_run(uint8_t *ip, const uint8_t *ip_bound, const uint8_t *ref) {


Why do you get rid of inline keyword. Is it unnecessary on all modern compilers?

This change was unintentional. I will revert it. And yes, modern compilers ignore this keyword, I usually use __attribute__((force_inline)) instead.

FrancescAlted · 2018-02-19T15:56:28Z

tests/test_shuffle_roundtrip_avx2.c

-  #pragma message("AVX2 shuffle tests not enabled.")
-  #else
-  #warning AVX2 shuffle tests not enabled.
-  #endif


Why you don't want the AVX2 not being tested messages to appear?

It breaks the build if we want to enable -Werror at a later time. Do you feel strongly for keeping it?

Well, I am just used to see it. If I see it, I know that AVX2 is not being tested in the platform. Probably mainly useful during early stages of AVX2 support though, so +1.

FrancescAlted · 2018-02-19T16:03:00Z

CMakeLists.txt

-     endif(NOT CMAKE_C_FLAGS AND COMPILER_SUPPORT_SSE2)
+     if(COMPILER_SUPPORT_SSE2)
+         add_compile_options(-msse2)
+     endif(COMPILER_SUPPORT_SSE2)


Does add_compile_options() require a cmake > 2.8.10? This is the current minimum required.

Good catch, seems it was added in cmake 2.8.12. Should we require cmake >= 2.8.12 or should I revert my changes to CMakeLists.txt?

Fortunately it seems like Ubuntu trusty has 2.8.12, so let's make a bump. Could you do that in main CMakeLists.txt?

estan · 2018-02-19T18:23:11Z

CMakeLists.txt

@@ -251,15 +251,18 @@ if(DEACTIVATE_AVX2)
 endif()

 # flags
-# @TODO: set -Wall
+# Set -Wall and other useful warning flags.
+if(CMAKE_C_COMPILER_ID STREQUAL GNU OR CMAKE_C_COMPILER_ID STREQUAL Clang OR CMAKE_C_COMPILER_ID STREQUAL Intel)


You could also toss AppleClang in there for good measure. It's how Apple's own version of Clang is identified.

Instead of the if, you could use generator expressions like:

add_compile_options( "$<$<CXX_COMPILER_ID:GNU>:-Wall;-Wwrite-strings;-Wno-unused>" "$<$<CXX_COMPILER_ID:Clang>:-Wall;-Wwrite-strings;-Wno-unused>" "$<$<CXX_COMPILER_ID:AppleClang>:-Wall;-Wwrite-strings;-Wno-unused>" "$<$<CXX_COMPILER_ID:Intel>:-Wall;-Wwrite-strings;-Wno-unused>" )

But I don't know if that's any prettier? Best-looking would be if the 3.3+ feature if ("bar" IN_LIST _list) could be used I think, but that's not in 2.8.

If you keep it the way it is, perhaps move the add_compile_option to inside the if block just below (the one for SSE2)? (since the conditions are the same).

In fact, thinking a bit, don't take the generator expressions as a serious suggestion. It's just cryptic, while anyone who reads the CMakeLists.txt probably just want to get on with what they were doing and not learn all about CMake's warts :)

Generator expressions seems to be primarily for when you need information that is only available at CMake's final "generation" step, and the compiler ID is not such a thing.

estan · 2018-02-19T18:23:46Z

CMakeLists.txt

+# Set -Wall and other useful warning flags.
+if(CMAKE_C_COMPILER_ID STREQUAL GNU OR CMAKE_C_COMPILER_ID STREQUAL Clang OR CMAKE_C_COMPILER_ID STREQUAL Intel)
+    add_compile_options(-Wall -Wwrite-strings -Wno-unused-function)
+endif(CMAKE_C_COMPILER_ID STREQUAL GNU OR CMAKE_C_COMPILER_ID STREQUAL Clang OR CMAKE_C_COMPILER_ID STREQUAL Intel)


Repeating the condition in the endif(..) is no longer necessary I think, so this could be just endif().

Repeating the condition in the endif(...) was just a way to see at a glance where the condition block finished. This is more useful in nested conditions, and we could remove it from here (not a big deal though).

@FrancescAlted Aha, I thought it was a left-over from < 2.6 times, when it was mandatory. Yes, no big deal, and could just as well be kept.

estan · 2018-02-19T18:29:39Z

@kalvdans Looks like some nice cleanups. Just a couple of comments, but 👍 from me.

Christian Häggström added 4 commits February 17, 2018 20:56

Turn on useful warnings from gcc

910d93b

Fix code that gcc warns for. Remove unused variables, const poisoning, and use declare functions (void) instead of ().

Fix warnings on macintosh and errors with visual C

571a3ed

Give up on warning for unused functions

327e717

Function definitions in header files makes some functions unused in some compilation units.

Revert changes that had no effect.

9b0bdcb

Christian Häggström added 2 commits February 19, 2018 10:36

Use builtin function for compiler flags

a2242c6

Use add_compile_options() instead of fiddling with variables. The options will apply to both C and C++ so I had to drop -Wstrict-prototypes.

Revert Windows flags + no need to quote gcc flags

c749a31

I better not touch Windows flags since I can't easily test out the changes locally.

FrancescAlted reviewed Feb 19, 2018

View reviewed changes

Christian Häggström added 2 commits February 19, 2018 18:26

Bump cmake version to get add_compile_options()

8d6b7c0

Revert unintentional change.

7f3e2de

estan reviewed Feb 19, 2018

View reviewed changes

FrancescAlted merged commit b569be2 into Blosc:master Feb 22, 2018

stevengj mentioned this pull request Sep 11, 2019

const-qualify static strings Blosc/c-blosc2#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on useful warnings from gcc #218

Turn on useful warnings from gcc #218

kalvdans commented Feb 17, 2018

kalvdans commented Feb 18, 2018

kalvdans commented Feb 19, 2018

FrancescAlted Feb 19, 2018

kalvdans Feb 19, 2018

FrancescAlted Feb 19, 2018

kalvdans Feb 19, 2018

FrancescAlted Feb 19, 2018

FrancescAlted Feb 19, 2018

kalvdans Feb 19, 2018

FrancescAlted Feb 19, 2018

estan Feb 19, 2018

estan Feb 19, 2018 •

edited

Loading

estan Feb 19, 2018

FrancescAlted Feb 19, 2018

estan Feb 19, 2018 •

edited

Loading

estan commented Feb 19, 2018

		@@ -79,7 +79,7 @@



		static inline uint8_t get_run(uint8_t ip, const uint8_t ip_bound, const uint8_t ref) {
		static uint8_t get_run(uint8_t ip, const uint8_t ip_bound, const uint8_t ref) {

Turn on useful warnings from gcc #218

Turn on useful warnings from gcc #218

Conversation

kalvdans commented Feb 17, 2018

kalvdans commented Feb 18, 2018

kalvdans commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estan Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estan Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

estan commented Feb 19, 2018

estan Feb 19, 2018 •

edited

Loading

estan Feb 19, 2018 •

edited

Loading