Don't add -Winline for WARNS=6
ClosedPublic
Actions

Authored by arichardson on Mar 12 2021, 7:02 PM.

Details

Reviewers

jrtc27
jhb
imp

Commits

rGf32309564547: Don't add -Winline for WARNS=6
rGc8c62548bffb: Don't add -Winline for WARNS=6

Summary

This warning is very rarely useful (inline is a hint and not mandatory).
This flag results in many warnings being printed when compiling C++
code that uses the standard library with GCC.

This flag was originally added in back in r94332 but the flag is a no-op
in Clang ("This diagnostic flag exists for GCC compatibility, and has no
effect in Clang"). Removing it should make the GCC build output slightly
more readable.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

arichardson created this revision.Mar 12 2021, 7:02 PM

Herald added subscribers: emaste, imp, bdrewery. · View Herald TranscriptMar 12 2021, 7:02 PM

arichardson requested review of this revision.Mar 12 2021, 7:02 PM

Harbormaster completed remote builds in B37798: Diff 85669.Mar 12 2021, 7:03 PM

jrtc27 added inline comments.Mar 12 2021, 7:04 PM

share/examples/etc/make.conf
97 ↗	(On Diff #85669)	This one probably should stay or the entire thing rewritten

arichardson added inline comments.Mar 12 2021, 7:06 PM

share/examples/etc/make.conf
97 ↗	(On Diff #85669)	sure will drop

Looks fine to me in general.

crypto/openssh/regress/misc/kexfuzz/Makefile
57 ↗	(On Diff #85669)	These are upstream files in openssh that I think we don't use in FreeBSD itself and should probably be left as-is.
tools/regression/capsicum/syscalls/Makefile
5	I wonder how stale these warning flags are and why this isn't just using bsd.prog.mk. Not a problem for this review to solve though.

arichardson added inline comments.Mar 12 2021, 7:29 PM

crypto/openssh/regress/misc/kexfuzz/Makefile
57 ↗	(On Diff #85669)	Thanks, wasn't sure about those.

Drop unncessary changes

Harbormaster completed remote builds in B37799: Diff 85670.Mar 12 2021, 7:31 PM

This warning very rarely useful (it's a hint and not mandatory) and it

You're missing an "is", and the "it" is a bit confusing (grammatically it should refer to the warning), I'd suggest expanding to "inline is a hint".

arichardson edited the summary of this revision. (Show Details)Mar 12 2021, 7:40 PM

arichardson edited the summary of this revision. (Show Details)Mar 12 2021, 7:42 PM

arichardson edited the summary of this revision. (Show Details)

These look good to me (one I'm torn on, but I'm good either way now that I've given my feedback).
The bootloader ones I explicitly approve and thing they are right
The others look good to me.

stand/i386/boot2/Makefile
35	Unconditional elimination is fine. We wanted this in yesteryear to help us understand bloat, but it's not been that helpful lately.
stand/i386/isoboot/Makefile
32	Same as with other bootloader stuff. I'd like to get rid of the -Wno-pointer-sign, but that's another day
sys/conf/kern.mk
51	I'm torn on this one staying or going... If someone has extra flags that include -Winline, is it a bug for the kernel? Or should the kernel compile with the warnings... I'm unsure.

This revision is now accepted and ready to land.Mar 12 2021, 8:10 PM

arichardson added inline comments.Mar 14 2021, 2:40 PM

sys/conf/kern.mk
51	We can keep this one to prevent people from getting -Werror failures. But then again maybe we should just say "don't add -Winline to CFLAGS".

jrtc27 added inline comments.Mar 14 2021, 3:54 PM

sys/conf/kern.mk
51	-Winline doesn't warn about functions in system headers, so anything out-of-tree should be entirely unaffected, with the warnings only being about _their_ code.

jhb added inline comments.Mar 19 2021, 6:17 PM

sys/conf/kern.mk
51	I think this one can go. Someone has to explicitly add `-Winline` while using GCC. If they want that, then they can deal with if they want to treat the new warning as an error or not. This list is about how we treat warnings we explicitly enable (or don't disable) in the tree.