4 ms·

-Wall sure (also, it should really enable all warnings, just as the name implies. Currently it doesn't) -Werror, I'm not sure. During development and on CI sys

by pilif 10y ago

-Wall sure (also, it should really enable all warnings, just as the name implies. Currently it doesn't) -Werror, I'm not sure. During development and on CI systems it makes sense. But if you intend to ship source code, it's probably best left off. Other people might be using different compilers that emit warnings for things yours didn't. You don't want the build to fail for those people.

jacquesm 10y ago
> Other people might be using different compilers that emit warnings for things yours didn't. You don't want the build to fail for those people. Actually, you do. That way those things will be fixed and hopefully your inclusion of their fixes is a well motivated PR away. Switching -Werror off should be a decision made with great care and understanding of what's going on under the hood. If there are platform specific issues then people on those platforms will be the ones in the best position to determine if such a warning is actually an error or not.
- simias 10y ago
  I want to agree with you but in practice I can't really imagine switching -Werror outside of dev builds. The problem is that different compilers have different warnings and if your code is meant to be portable it's going to be a serious pain to avoid all the warnings all the time. Having compilation break for a user because of a spurious warning due to a different compiler (or different compiler version) would make it a lot more painful to maintain. For instance I started maintaining an old C++ codebase that generates a bunch of warnings when build with a modern g++, code like this: #define M "toto" const char *s = "tata"M; Generates warnings like: warning: invalid suffix on literal; C++11 requires a space between literal and string macro [-Wliteral-suffix] Is it helpful? Sure. It is worth breaking the build for anybody using a modern g++ until I manage to fix all occurrences and release a new version? I don't think so. An other example is building code with -Wextra which warns you when some comparisons become "nops". For instance: int toto(int i) { if (i <= 0xffff) return 1; return 0; } On most modern machines this will build without a hitch, but if you compile on a 16bit machine you'll get a warning saying: warning: comparison is always true due to limited range of data type In my experience this particular warning is generally not a problem. On the other hand the following code: int i = 0xabcdef; Will generate the following warning on 16bit architectures: warning: integer constant is too large for its type And that's potentially a lot nastier and should be an error IMO. So I'd say the main problem is that C and C++ are way too permissive by default, things like comparison with no side effects ought to be errors, not warnings. And there are a bunch of others, for instance why on earth are these not errors : warning: no return statement in function returning non-void warning: ‘i’ is used uninitialized in this function So -Werror is just killing a fly with a bazooka IMO.
  - joosters 10y ago
    warning: comparison is always true due to limited range of data type Surely this is a serious problem on 16 bit builds; the comparison implies that larger values are expected to be stored in this variable, the warning is a strong hint that the (size of the) types are incorrect.
    - simias 10y ago
      In my experience it's generally not the case, you generally do these tests if you're about to truncate a value or do sanity checks. If the value is too big to fit the variable then it should've been caught earlier, when the value was loaded in the first place (i.e. strtol or whatever).

by pilif 10y ago

jacquesm 10y ago

> Other people might be using different compilers that emit warnings for things yours didn't. You don't want the build to fail for those people. Actually, you do. That way those things will be fixed and hopefully your inclusion of their fixes is a well motivated PR away. Switching -Werror off should be a decision made with great care and understanding of what's going on under the hood. If there are platform specific issues then people on those platforms will be the ones in the best position to determine if such a warning is actually an error or not.

simias 10y ago
I want to agree with you but in practice I can't really imagine switching -Werror outside of dev builds. The problem is that different compilers have different warnings and if your code is meant to be portable it's going to be a serious pain to avoid all the warnings all the time. Having compilation break for a user because of a spurious warning due to a different compiler (or different compiler version) would make it a lot more painful to maintain. For instance I started maintaining an old C++ codebase that generates a bunch of warnings when build with a modern g++, code like this: #define M "toto" const char *s = "tata"M; Generates warnings like: warning: invalid suffix on literal; C++11 requires a space between literal and string macro [-Wliteral-suffix] Is it helpful? Sure. It is worth breaking the build for anybody using a modern g++ until I manage to fix all occurrences and release a new version? I don't think so. An other example is building code with -Wextra which warns you when some comparisons become "nops". For instance: int toto(int i) { if (i <= 0xffff) return 1; return 0; } On most modern machines this will build without a hitch, but if you compile on a 16bit machine you'll get a warning saying: warning: comparison is always true due to limited range of data type In my experience this particular warning is generally not a problem. On the other hand the following code: int i = 0xabcdef; Will generate the following warning on 16bit architectures: warning: integer constant is too large for its type And that's potentially a lot nastier and should be an error IMO. So I'd say the main problem is that C and C++ are way too permissive by default, things like comparison with no side effects ought to be errors, not warnings. And there are a bunch of others, for instance why on earth are these not errors : warning: no return statement in function returning non-void warning: ‘i’ is used uninitialized in this function So -Werror is just killing a fly with a bazooka IMO.
- joosters 10y ago
  warning: comparison is always true due to limited range of data type Surely this is a serious problem on 16 bit builds; the comparison implies that larger values are expected to be stored in this variable, the warning is a strong hint that the (size of the) types are incorrect.
  - simias 10y ago
    In my experience it's generally not the case, you generally do these tests if you're about to truncate a value or do sanity checks. If the value is too big to fit the variable then it should've been caught earlier, when the value was loaded in the first place (i.e. strtol or whatever).