On Fri, May 29, 2020 at 03:43:06PM +0200, Tim Düsterhus wrote: > Willy, > > Am 29.05.20 um 14:57 schrieb Willy Tarreau: > > Hi Tim, > > > > On Fri, May 29, 2020 at 02:35:49PM +0200, Tim Duesterhus wrote: > >> Hi there, > >> > >> These two patches re-enable some build warnings in the Makefile. The first > >> one > >> only enables those that did not trigger for me at all. > > > > OK, I agree with not removing warnings that don't trigger. > > Should the 'not' in front of 'removing' be removed in that sentence?
It's the same :-) I meant that I'm fine with not adding explicit "-Wno-XXX" options that are meant to disable warnings if we figure that such warnings do not appear anymore. > >> The second one re-enables -Wimplicit-fallthrough. I could not adjust all > >> places, because I was not sure whether they work as intended. > >> > >> I've Cc't Willy (stream_interface, pattern) and William (cli), because they > >> where the ones that wrote the switch statements. I've also added Ilya, > >> because > >> applying the second patch will most likely fail the Travis build. > > > > Indeed if there are so few left, it might be worth trying to address > > them and re-enabling the warning. > > It's also super easy to silence any false positives of that warning by > adding a comment. In fact, to be honest, that might be one of the warnings that I hate the most because it's the first time in history that a compiler reads your comments to decide to emit warnings. And in addition some people are having trouble with writing english or might even be prevented from writing english comments in their code for various local policies, some suffer from dyslexia and cannot spell "through", and all of them are annoyed by such a warning. The right way to do it would instead be to have a __builtin_fall_through() or something like this. Gcc developers recently figured it an proposed __attribute__((fallthrough)) which is already a great improvement over the stupid principle of reading our comments. A further improvement would be to support placing that attribute on the switch() instruction to cover the whole block, because obviously the person who invented this warning has never ever written a state machine. But over time our code started to become more cautious about such cases and if we are now almost ready to switch to it I think we can do it and secure the situation for the future. > >> I'd say that the one in 'pattern' is a bug and that the other two just > >> need a > >> /* fall through */ comment. > > > > No opinion yet since I've not read that code for a while. However I'd like > > the code to be fixed before we enable the warning, as I absolutely don't > > want to see a build warning during regular development. We encourage > > building with -Werror and this would force developers to remove it which > > would be a huge step backwards! > > I am fine with that. That's why I put all the involved developers in Cc. That's what I understood :-) > > That reminds me, I seem to have noticed some warnings a few months ago > > when building for i386, maybe something related to printf formats having > > %l when ints were used instead. It's been a long time and I don't even > > remember if we've fixed that. It just happens to be the right period in > > the development cycle to start to address such things. The cleaner the > > better. > > Possibly also have a look at -Wclobbered. It only triggers at two places > within Lua with something regarding 'setjmp'. It might be possible to > re-enable that one as well, but I don't understand that code. I didn't remember about that one. Longjmp/setjmp are a hell to follow, both for humans and compilers, so it's possible that there's a bug just like it's possible that the compiler is wrong! Definitely worth having a look at indeed. Willy
