Opened 3 years ago

Closed 3 years ago

#19044 closed enhancement (implemented)

Turn on --enable-gcc-warnings-advisory by default

Reported by: special Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: TorCoreTeam201605, 029-nickm-says-yes
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: special Sponsor:

Description

I've had multiple patches fail due to missing the --enable-gcc-warnings flag in builds. I've also missed really important problems that these warnings would tell me about.

Turning on --enable-gcc-warnings by default is probably not a good choice, because GCC warnings vary quite a lot between versions and -Werror will cause these new warnings to break builds.

Without -Werror, there's no harm in enabling all of these warnings by default, and this makes us more likely to notice important warnings. We should do it.

I suggest:

  1. Make the behavior of --enable-gcc-warnings-advisory default when supported by the compiler
  2. Add an --enable-fatal-warnings option to turn on -Werror
  3. Alias --enable-gcc-warnings to --enable-fatal-warnings for backwards compatibility
  4. Deprecate or ignore --enable-gcc-warnings-advisory
  5. Optionally, we could have a --disable-extra-warnings flag, if there's a case where someone wants to turn off these warnings

Child Tickets

Change History (9)

comment:1 Changed 3 years ago by special

Keywords: 029-proposed TorCoreTeam201605 added

comment:2 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:3 Changed 3 years ago by nickm

Points: 0.05

comment:4 Changed 3 years ago by isabela

Points: 0.050.1

comment:5 Changed 3 years ago by nickm

Keywords: 029-proposed removed

Calling these included in 0.2.9 because the (remaining) amount of work should be very small.

comment:6 Changed 3 years ago by nickm

Actual Points: 0.1
Owner: set to nickm
Status: newaccepted

I've done the easy version. Now --enable-gcc-warnings-advisory is on by default. --disable-gcc-warnings-advisory is possible. --enable-fatal-warnings sets -Werror if it can. And --enable-gcc-warnings is an alias for --enable-fatal-warnings.

comment:7 Changed 3 years ago by nickm

Status: acceptedneeds_review

Patches in branch ticket19044

comment:8 Changed 3 years ago by special

Reviewer: special
Status: needs_reviewmerge_ready

LGTM. I won't be surprised if we find out that one of these flags isn't accepted by all GCC versions and breaks compilation somewhere, but it's reasonably defensive about this already.

We should look at making that list of warnings longer at some point.

comment:9 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged, and thanks for the review!

The 'make the list of warnings longer' ticket is now #19180.

Note: See TracTickets for help on using tickets.