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? >> 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. >> 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 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. Best regards Tim Düsterhus
