GNOME Bugzilla – Bug 732531
generate pragmas to suppress warnings
Last modified: 2018-05-22 15:12:22 UTC
Created attachment 279642 [details] [review] codegen: generate pragmas to suppress warnings Vala code generates a lot of warnings which obscure the useful warnings. For example, after adding this patch I noticed bug #732530 while compiling Bump due to a legit C warning from clang which I have been overlooking for a long time.
Created attachment 279644 [details] [review] codegen: generate pragmas to suppress warnings Forgot to include ccode/valaccodepragma.vala
No I prefer to fix them rather than suppress.
That sounds good, but these have been around for a *long* time without (AFAIK) even a plan to address the issue. I'd rather see them suppressed, and if we come up with a fix later on we can always remove the pragmas. Also, the deprecated declarations ones aren't even possible to fix--they should just be suppressed since valac should already be emitting deprecation warnings, it doesn't make sense to have the CC do it to.
(In reply to comment #3) > That sounds good, but these have been around for a *long* time without (AFAIK) > even a plan to address the issue. I'd rather see them suppressed, and if we > come up with a fix later on we can always remove the pragmas. Not a reason to add an hack to the compiler. Just pass the flags to gcc to suppress warnings. > Also, the deprecated declarations ones aren't even possible to fix--they should > just be suppressed since valac should already be emitting deprecation warnings, > it doesn't make sense to have the CC do it to. That's ok, since gcc does not allow per-symbol suppress of the warning.
(In reply to comment #4) > (In reply to comment #3) > > That sounds good, but these have been around for a *long* time without (AFAIK) > > even a plan to address the issue. I'd rather see them suppressed, and if we > > come up with a fix later on we can always remove the pragmas. > > Not a reason to add an hack to the compiler. People are passing -w to the C compiler instead of just disabling the specific warnings that Vala tends to trigger, meaning they aren't seeing the legitimate warnings. Or they aren't passing anything and are just ignoring C warnings from Vala code, losing the legitimate warnings in the noise. People don't read through all the warnings to try to figure out which ones are useful and which ones aren't. And even if they do at first, they aren't going to do it every time they upgrade a dependency or their Vala compiler. Asking them to deal with all these warnings is just unrealistic. > Just pass the flags to gcc to > suppress warnings. This patch just does that for people. It makes it easy to get good warnings while filtering out the noise. > > Also, the deprecated declarations ones aren't even possible to fix--they should > > just be suppressed since valac should already be emitting deprecation warnings, > > it doesn't make sense to have the CC do it to. > > That's ok, since gcc does not allow per-symbol suppress of the warning. False positives are not okay. That said, this isn't actually true... you could generate something like #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" // Code which uses symbol known by vala to be deprecated #pragma GCC diagnostic pop Of course, in reality it would be a mess of preprocessor directives to account for compilers other than GCC. But I take it back, it would be possible to fix them. Actually, they are probably much easier to fix than const or incompatible pointer types. I have no idea how to fix the const mismatch issue. For pure Vala code it might be possible, but we would have to add *lots* of annotations to VAPIs which would make them much more unpleasant to read and painful to write. Incompatible pointers would also require lots of annotations, though less than const. The problem here is that if we start automatically casting stuff it is going to break code... like anything where we create a fake delegate type in a VAPI because the C API is missing a typedef. Detecting cases where the C type isn't what is expected in the GIR parser (for example, if there is a (type) annotation) would, I think, require some pretty significant changes in the compiler, since AFAIK there is currently no good way to figure out what the expected C type of a Symbol is without going through the codegen. Until someone has time to fix these properly I don't see why we shouldn't try to make things a bit better for our users. Yes, the #pragma thing is a hack, but I think the huge reduction is false positives is more than worth the small reduction of false negatives. I would be okay having this be disabled by default and hide it behind a flag, though I would prefer it be enabled by default with a flag to turn it off.
Can we add a --disable-emit-pragmas?
Sure. As I said, "I would be okay having this be disabled by default and hide it behind a flag, though I would prefer it be enabled by default with a flag to turn it off."
We can enable it by default for me.
Ok, I'll update the patch as soon as I have a few minutes. How about --disable-warning-suppression instead, though?
*warnings. Ok.
Sorry, that's confusing about whether it's vala warnings or C warnings. What about --enable-ccode-warnings .
These days, Vala creates C code that compiles with much less warnings. Would you still think these pragmas are necessary?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/458.