Opened 3 years ago

Closed 3 years ago

#19139 closed enhancement (implemented)

Let Autoconf handle enabling C and POSIX extensions

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

The comment on src/common/compat.c:18 suggests using AC_USE_SYSTEM_EXTENSIONS. The attached patch does just that.

Defining the preprocessor macros globally ensures the extensions are always turned on. Running GCC in pedantic mode tells me that there are instances where the code assumes the extensions are enabled while missing the proper preprocessor macros. FWICT it currently just works because GCC and its headers turn their own extensions on by default.

The comment suggestion about maybe removing the environ configure check has not been researched.

Child Tickets

Attachments (2)

0001-Use-the-Autoconf-macro-AC_USE_SYSTEM_EXTENSIONS.patch (6.8 KB) - added by cypherpunks 3 years ago.
0001-Use-the-Autoconf-macro-AC_USE_SYSTEM_EXTENSIONS.2.patch (7.2 KB) - added by cypherpunks 3 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 3 years ago by cypherpunks

Status: newneeds_information

FWIW the removal of the existing macros in the patch was done by grepping for #define _ and removing instances manually according to the list in the Autoconf manual.

And now for the environ variable. Research into the POSIX standard and the manpages of Debian, OpenBSD and Mac OS X shows that the external environ variable is declared in unistd.h. To be on the safe side the declaration in compat.c can be kept but it does not need to be conditionally because external variables can be declared multiple times (as long as their declaration is the same). Therefore the environ configure check can be removed. As this change is unrelated to the ticket description, I'll open a new ticket and refer back to this comment.

One remaining question: does the initial patch need a changes file? If so, I'm happy to write one.

comment:2 Changed 3 years ago by cypherpunks

The removal of the environ configure check is now tracked in #19142.

comment:3 Changed 3 years ago by cypherpunks

Status: needs_informationneeds_review

The previous patch was not cleanly merging to master anymore so I've rebased it. Also added a changes file so it's ready to be reviewed.

comment:4 Changed 3 years ago by nickm

lgtm. Applying; thanks!

comment:5 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-final
Resolution: implemented
Reviewer: nickm
Status: needs_reviewclosed
Type: defectenhancement
Note: See TracTickets for help on using tickets.