This is an archive of the discontinued Mercurial Phabricator instance.

cdatapack: move feature macros before including cdatapack.h
ClosedPublic

Authored by quark on Aug 29 2017, 8:25 AM.
Tags
None
Subscribers

Details

Summary

On Arch Linux, glibc 2.25-7, cdatapack.h includes stdint.h, which
includes bits/libc-header-start.h, which includes features.h. Therefore
_DEFAULT_SOURCE and _BSD_SOURCE must be defined before including
cdatapack.h.

ntoh_data_offset was moved to .c to make sure .c is the only direct
and indirect user of be64toh.

Test Plan

Make sure make local works on Arch Linux.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Aug 29 2017, 8:25 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 29 2017, 8:25 AM
ryanmce accepted this revision.
This revision is now accepted and ready to land.Aug 29 2017, 9:55 AM
simpkins added inline comments.Aug 29 2017, 3:49 PM
cdatapack/cdatapack.c
10–19

If cdatapack.h requires these settings it seems like these really should be
defined in cdatapack.h. Otherwise anyone else including cdatapack.h in the
future will end up having to track down the issue all over again, and then copy
and paste this code into their own file too.

quark added inline comments.Aug 29 2017, 4:07 PM
cdatapack/cdatapack.c
10–19

It seems cdatapack.c is the only user of the function. I'll move ntoh_data_offset definition to .c too to make it clear.

quark edited the summary of this revision. (Show Details)Aug 29 2017, 4:12 PM
quark updated this revision to Diff 1412.
quark added a comment.EditedAug 29 2017, 4:15 PM

Because the feature macros must be defined at the very beginning. I think it's fine to have them in .c - it seems more clear to me that the marcos will be effective as desired. Putting them in cdatapack.h is okay in this particular case. But I think when people add more or reorder includes (ex. adding #include "abc.h" and put it naturally before cdatapack.h), they may run into surprises.

This revision was automatically updated to reflect the committed changes.