Skip to content

Migrate Includes to Relative Paths#2103

Merged
felixhandte merged 10 commits intofacebook:devfrom
felixhandte:relative-includes
May 6, 2020
Merged

Migrate Includes to Relative Paths#2103
felixhandte merged 10 commits intofacebook:devfrom
felixhandte:relative-includes

Conversation

@felixhandte
Copy link
Contributor

This removes the need for adding our subdirectories to the compiler's include path (via -Ilib, -Ilib/common, etc.), both for our own builds, and for those who copy the zstd source in their own source tree and build using their own build system.

This PR addresses #1998.

Tested with make and cmake. Will let contbuild test the other build systems.

@felixhandte felixhandte force-pushed the relative-includes branch 2 times, most recently from cb393cc to e7cfa86 Compare May 1, 2020 22:09
@felixhandte felixhandte force-pushed the relative-includes branch from e7cfa86 to b48f6c7 Compare May 4, 2020 19:21
@terrelln
Copy link
Contributor

terrelln commented May 4, 2020

Looks like the single file decoder is still failing

Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the tests pass though.

@Cyan4973
Copy link
Contributor

Cyan4973 commented May 6, 2020

Great work @felixhandte !

@felixhandte felixhandte merged commit ad8dbae into facebook:dev May 6, 2020
@cwoffenden
Copy link
Contributor

The changes to the single-file generator script inlines files multiple times. For example, xxhash.h is inlined once as common/xxhash.h, then as compress/../common/xxhash.h, etc.

The resulting binary file is the same, and the include guards do their job against these multiple copies, but the resulting .c file for the decoder is over twice the size.

@Cyan4973 Cyan4973 mentioned this pull request May 21, 2020
orivej added a commit to orivej/zstd that referenced this pull request May 22, 2020
Cyan4973 added a commit that referenced this pull request May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants