5 ms·

Cleaning up X server warnings

45 points by ruda 13y ago

raverbashing 13y ago
Interesting. Interesting how the -Wcast-qual warnings are unfixable, so it shouldn't exist. Damned if you do, damned if you don't. A lot of warnings seem to fall on this. Or warnings that would make sense on stricter languages but doesn't make sense on C exactly because of what you can do with C, like, for example, reading serialized data, then casting it to (MyStructure *) And a little bit offtopic, but GTk have some unfixable warnings as well, something like "blah is not implemented in this platform" so, if I have to use this, but BLAH is not implemented, thanks for warning me, but shut up if I can't fix it
- joosters 13y ago
  I thought that the issue is the const-ness of the variables, and nothing to do with casting void* ->sometype* Essentially, they are making use of const pointers to ensure that the code doesn't change the data. (gcc would throw a warning if you did). BUT: the problem comes when you want to free() the data. A strict interpretation of C would be that you can't free() something that's const, because it clearly is altering the data. However, you have to free the data sometime or other... A possible workaround is to write a freeconst() macro that does the cast to void* and calls free(), and wrap this macro in the 'temporarily disable this warning #pragma'. This way, you only need to turn off the warning in one place in your code.
  - mtklein 13y ago
    On a related note, if you're using new and delete you can simply delete a pointer-to-const without jumping through any const_cast hoops, though I often wish I had a way of blocking that for APIs where the callee doesn't take ownership of a const Foo*.
    - joosters 13y ago
      Is that actually a standard though? ISTR some compilers complaining if you use delete with a const pointer (sorry, this is from ages ago, I cannot recall which compiler...) You can make a fair argument that using delete with a const pointer is a type error, after all the data can hardly be considered const if it has just been eradicated...
  - anonymouscowar1 13y ago
    The BSD solution to this problem is __DECONST(), a macro which basically casts through `uintptr_t`. #define __DECONST(type, var) ((type)(uintptr_t)(const void*)(var))
- joosters 13y ago
  I hate those GTK runtime errors. When I run a gtk app from a shell & find pages of warnings, I lose all faith in the app.
- unwind 13y ago
  Reading serialized data and then casting it into a structure type is ill-defined, very bad practice, and there's nothing in C that tells you it's a good or safe thing to be doing. So, please don't do that. The exact layout of a structure in memory is up to the compiler, and can easily change with compiler options, even if the compiler itself and the target architecture and platform remain the same. An externally-visible serialization format should of course be stable, and not depend on the compiler used to build the code, or the hardware platform which might have ideas of how various fields should be aligned (or even how to order the bytes in integers larger than 8 bits). Serializing data is something you do, just blindly copying the run-time representation that you have to an external medium is not serialization. You should do it field by field, with a well-defined format chosen for each field.
  - vidarh 13y ago
    All of that is true unless it is, say, cache data that you can afford to ditch if the format has changed, and you validate it after reading (say with a carefully structured "magic" value).
  - viraptor 13y ago
    That's the theory, sure. But in practice, if you know all your targets support the right "#pragma pack" and "__attribute__((packed))" and other markers that you use - what's wrong with making use of them?
    - joosters 13y ago
      Also, if you're passing data between two processes (e.g. your code fork()d a child), you can be sure that the data format is going to match, no packing required. (still need to ensure alignment in memory though)
      - mikeash 13y ago
        This is not necessarily true. Lots of systems support running 32-bit and 64-bit binaries simultaneously, and structs won't necessarily be portable between the two. Some versions of Mac OS X even supported running 32-bit PowerPC binaries (in an emulator, but supported directly by the OS) next to both 32-bit and 64-bit x86 binaries, so you could not only get size mismatches, but also endian mismatches. This does not apply to simply forking, of course, but it does apply to the "two processes" case in general.
- sophacles 13y ago
  The notion of warnings is not that it the behavior is broken, or even wrong, but rather dangerous. A good solution would be to include a pragma or such that that would allow programmers to say "I recognize this is dangerous, but it is correct". Even better would be a pragma that allows you to say the above, and also point to a test/test_suit that verifies that the specific case is still working right and warns on "something has changed, test foo no longer holds, here's the associated warning as a hint".
  - raverbashing 13y ago
    Well, some warnings are valid, but yeah, the "I recognize this is dangerous, but it is correct" makes perfect sense.
- IsTom 13y ago
  If you're not careful and not on x86-likes, then reading serialized data like that unaligned values might cause errors.
tenfingers 13y ago
It always surprised me that GCC has no source-level way of controlling compiler warnings. Many programmers (and gcc apparently) live with the assumption that compiler warnings must always be fixed. This is largely false. Compiler warnings are there to help with additional information, not to be something to fix. There are many cases where the compiler issues a warning which must be ignored. Many compilers support pragmas to silence a specific warning class in a specific point in the code. IMHO it's a much better approach to silence just a single warning instance where you know the compiler is getting the wrong assumption than silencing a whole warning class like the article suggests. But it seems that in GCC you have no choice. Instead, code written with/for gcc often silences the warning by inserting some dummy code. For instance, the "unitialized warning" is usually fixed by assigning the variable to itself. It's shitty to look at, and someone that doesn't know this trick might wonder why this is being done. I've reported a feature request many years ago, and it was quickly closed...
- noselasd 13y ago
  gcc has this capability: http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html I recon you meant unused variables (as assinging an unitialized variable to itself would beat the purpose) you can use __attribute__((unused)) on it - normally #defined to a macro.
- mitchty 13y ago
  I thought thats exactly what these are for: http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html Also work for clang like so (replace clang with GCC for its version): #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wsomeignoredwarning" triggered by this code #pragma clang diagnostic pop
- anonymouscowar1 13y ago
  Sure, sometimes they warn about totally valid behaviors. The "all warnings must be fixed" dogma arises not out of the belief that all warnings are errors, but rather that the value of having compiler warnings catch bugs early is well worth the cost of changing code style to conform to the compiler's warnings suite.
IgorPartola 13y ago
I always wondered why so many FOSS projects always generate warnings. I always assumed that the large mature projects like the X server did this because they were performing some crazy optimization that the compiler was too naive to know about. I never had to do this myself, so I thought that the pro's used their arcane knowledge to eek out an extra 0.1% performance out of something for the benefit of everyone. Turns out that the truth is so much less interesting.
- yxhuvud 13y ago
  Often, it will be because the projects are older than the warnings. And when the amount of warnings cross a threshold, they will not be fixed.
  - joosters 13y ago
    Yes, definitely this. If there are hundreds of warnings, there's no motivation to fix any of them. If there are none, hopefully no-one wants to add the code that creates one. Perhaps they now need to compile the project with warnings-as-errors to enforce clean code?

45 points by ruda 13y ago

Interesting. Interesting how the -Wcast-qual warnings are unfixable, so it shouldn't exist. Damned if you do, damned if you don't. A lot of warnings seem to fall on this. Or warnings that would make sense on stricter languages but doesn't make sense on C exactly because of what you can do with C, like, for example, reading serialized data, then casting it to (MyStructure *) And a little bit offtopic, but GTk have some unfixable warnings as well, something like "blah is not implemented in this platform" so, if I have to use this, but BLAH is not implemented, thanks for warning me, but shut up if I can't fix it

joosters 13y ago
I thought that the issue is the const-ness of the variables, and nothing to do with casting void* ->sometype* Essentially, they are making use of const pointers to ensure that the code doesn't change the data. (gcc would throw a warning if you did). BUT: the problem comes when you want to free() the data. A strict interpretation of C would be that you can't free() something that's const, because it clearly is altering the data. However, you have to free the data sometime or other... A possible workaround is to write a freeconst() macro that does the cast to void* and calls free(), and wrap this macro in the 'temporarily disable this warning #pragma'. This way, you only need to turn off the warning in one place in your code.
- mtklein 13y ago
  On a related note, if you're using new and delete you can simply delete a pointer-to-const without jumping through any const_cast hoops, though I often wish I had a way of blocking that for APIs where the callee doesn't take ownership of a const Foo*.
  - joosters 13y ago
    Is that actually a standard though? ISTR some compilers complaining if you use delete with a const pointer (sorry, this is from ages ago, I cannot recall which compiler...) You can make a fair argument that using delete with a const pointer is a type error, after all the data can hardly be considered const if it has just been eradicated...
- anonymouscowar1 13y ago
  The BSD solution to this problem is __DECONST(), a macro which basically casts through `uintptr_t`. #define __DECONST(type, var) ((type)(uintptr_t)(const void*)(var))
joosters 13y ago
I hate those GTK runtime errors. When I run a gtk app from a shell & find pages of warnings, I lose all faith in the app.
unwind 13y ago
Reading serialized data and then casting it into a structure type is ill-defined, very bad practice, and there's nothing in C that tells you it's a good or safe thing to be doing. So, please don't do that. The exact layout of a structure in memory is up to the compiler, and can easily change with compiler options, even if the compiler itself and the target architecture and platform remain the same. An externally-visible serialization format should of course be stable, and not depend on the compiler used to build the code, or the hardware platform which might have ideas of how various fields should be aligned (or even how to order the bytes in integers larger than 8 bits). Serializing data is something you do, just blindly copying the run-time representation that you have to an external medium is not serialization. You should do it field by field, with a well-defined format chosen for each field.
- vidarh 13y ago
  All of that is true unless it is, say, cache data that you can afford to ditch if the format has changed, and you validate it after reading (say with a carefully structured "magic" value).
- viraptor 13y ago
  That's the theory, sure. But in practice, if you know all your targets support the right "#pragma pack" and "__attribute__((packed))" and other markers that you use - what's wrong with making use of them?
  - joosters 13y ago
    Also, if you're passing data between two processes (e.g. your code fork()d a child), you can be sure that the data format is going to match, no packing required. (still need to ensure alignment in memory though)
    - mikeash 13y ago
      This is not necessarily true. Lots of systems support running 32-bit and 64-bit binaries simultaneously, and structs won't necessarily be portable between the two. Some versions of Mac OS X even supported running 32-bit PowerPC binaries (in an emulator, but supported directly by the OS) next to both 32-bit and 64-bit x86 binaries, so you could not only get size mismatches, but also endian mismatches. This does not apply to simply forking, of course, but it does apply to the "two processes" case in general.
sophacles 13y ago
The notion of warnings is not that it the behavior is broken, or even wrong, but rather dangerous. A good solution would be to include a pragma or such that that would allow programmers to say "I recognize this is dangerous, but it is correct". Even better would be a pragma that allows you to say the above, and also point to a test/test_suit that verifies that the specific case is still working right and warns on "something has changed, test foo no longer holds, here's the associated warning as a hint".
- raverbashing 13y ago
  Well, some warnings are valid, but yeah, the "I recognize this is dangerous, but it is correct" makes perfect sense.
IsTom 13y ago
If you're not careful and not on x86-likes, then reading serialized data like that unaligned values might cause errors.

tenfingers 13y ago

It always surprised me that GCC has no source-level way of controlling compiler warnings. Many programmers (and gcc apparently) live with the assumption that compiler warnings must always be fixed. This is largely false. Compiler warnings are there to help with additional information, not to be something to fix. There are many cases where the compiler issues a warning which must be ignored. Many compilers support pragmas to silence a specific warning class in a specific point in the code. IMHO it's a much better approach to silence just a single warning instance where you know the compiler is getting the wrong assumption than silencing a whole warning class like the article suggests. But it seems that in GCC you have no choice. Instead, code written with/for gcc often silences the warning by inserting some dummy code. For instance, the "unitialized warning" is usually fixed by assigning the variable to itself. It's shitty to look at, and someone that doesn't know this trick might wonder why this is being done. I've reported a feature request many years ago, and it was quickly closed...

noselasd 13y ago
gcc has this capability: http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html I recon you meant unused variables (as assinging an unitialized variable to itself would beat the purpose) you can use __attribute__((unused)) on it - normally #defined to a macro.
mitchty 13y ago
I thought thats exactly what these are for: http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html Also work for clang like so (replace clang with GCC for its version): #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wsomeignoredwarning" triggered by this code #pragma clang diagnostic pop
anonymouscowar1 13y ago
Sure, sometimes they warn about totally valid behaviors. The "all warnings must be fixed" dogma arises not out of the belief that all warnings are errors, but rather that the value of having compiler warnings catch bugs early is well worth the cost of changing code style to conform to the compiler's warnings suite.

IgorPartola 13y ago

I always wondered why so many FOSS projects always generate warnings. I always assumed that the large mature projects like the X server did this because they were performing some crazy optimization that the compiler was too naive to know about. I never had to do this myself, so I thought that the pro's used their arcane knowledge to eek out an extra 0.1% performance out of something for the benefit of everyone. Turns out that the truth is so much less interesting.

yxhuvud 13y ago
Often, it will be because the projects are older than the warnings. And when the amount of warnings cross a threshold, they will not be fixed.
- joosters 13y ago
  Yes, definitely this. If there are hundreds of warnings, there's no motivation to fix any of them. If there are none, hopefully no-one wants to add the code that creates one. Perhaps they now need to compile the project with warnings-as-errors to enforce clean code?