On Thu, Aug 29, 2019 at 8:13 PM Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote: > > Instead of the warning-[123] magic, let's accumulate compiler options > to KBUILD_CFLAGS directly as the top Makefile does. I think this makes > easier to understand what is going on in this file. > > This commit slightly changes the behavior, I think all of which are OK. > > [1] Currently, cc-option calls are needlessly evaluated. For example, > warning-3 += $(call cc-option, -Wpacked-bitfield-compat) > needs evaluating only when W=3, but it is actually evaluated for > W=1, W=2 as well. With this commit, only relevant cc-option calls > will be evaluated. This is a slight optimization. > > [2] Currently, unsupported level like W=4 is checked by: > $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown) > This will no longer be checked, but I do not think it is a big > deal. > > [3] Currently, 4 Clang warnings (Winitializer-overrides, Wformat, > Wsign-compare, Wformat-zero-length) are shown by any of W=1, W=2, > and W=3. With this commit, they will be warned only by W=1. I > think this is a more correct behavior since each warning belongs > to only one group. > > For understanding this commit correctly: > > We have 3 warning groups, W=1, W=2, and W=3. You may think W=3 has a > higher level than W=1, but they are actually independent. If you like, > you can combine them like W=13. To enable all the warnings, you can > pass W=123. This is shown by 'make help', but it is often missed > unfortunately. Since we support W= combination, there should not exist > intersection among the three groups. If we enable Winitializer-overrides > for W=1, we do not need to for W=2 or W=3. This is why I believe the > change [3] makes sense. > > The documentation says -Winitializer-overrides is enabled by default. > (https://clang.llvm.org/docs/DiagnosticsReference.html#winitializer-overrides) > We negate it by passing -Wno-initializer-overrides for the normal > build, but we do not do that for W=1. This means, W=1 effectively > enables -Winitializer-overrides by the clang's default. The same for > the other three. I wonder if this logic needs detailed commenting, > but I do not want to be bothered any more. I added comments. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> > Reviewed-by: Nathan Chancellor <natechancellor@xxxxxxxxx> > --- > > Changes in v2: > - Added comments and more commit log > > scripts/Makefile.extrawarn | 105 +++++++++++++++++++------------------ > 1 file changed, 54 insertions(+), 51 deletions(-) > Thanks for the v2. I am impressed by the very informative commit-log. But... I still miss relevant infos in the "kbuild-docs" aka in <Documentation/kbuild/kbuild.rst file> I also was not aware I can combine W=... settings like W=123 (W=132 and W=321 does the same I guess). In my little world W=3 should include W=1 and W=2. Such informations I would like to have in kbuild-docs. BTW, I mixed up kbuild-system with kconfig-system as I normally get in touch with the 2nd. Renaming the kbuild-variable is up to you. I am OK when you want to wait for Arnd's rework of extrawarn compiler options. I did change it in one of my patches treewide. - Sedat -
