Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure the source files #1679

Merged
merged 1 commit into from
Jul 19, 2019
Merged

Restructure the source files #1679

merged 1 commit into from
Jul 19, 2019

Conversation

ephiepark
Copy link
Contributor

This pull request reorganizes codes into different files.
This changes have the following side effect:

  • functions like ZSTD_noCompressLiterals, ZSTD_compressRleLiteralsBlock, ZSTD_selectEncodingType, ZSTD_buildCTable, ZSTD_encodeSequences are no longer static functions.

@terrelln
Copy link
Contributor

This is great! zstd_compress.c is gigantic

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jul 11, 2019

The change itself looks good to me.

Now, the main issue is around build scripts.
Some of them will have to be updated, because they can't just "grab the directory content", as Makefile does (or are not designed this way, and maybe they should be updated to do so).

This includes :

  • cmake
  • Visual Studio
  • meson

For the PR to pass, all build systems must be updated.
This is a major source of slow down.

At this stage, a quickfix is good enough.
though if you can find a way to make some of these build systems a bit more "generic",
this would be a great gain too.

@ephiepark
Copy link
Contributor Author

@Cyan4973 Thanks for the comment! I was confused about what was causing the issue. Let me look into them.

@ephiepark ephiepark force-pushed the dev branch 2 times, most recently from 30ee89a to 41e39c8 Compare July 11, 2019 22:48
@Cyan4973 Cyan4973 self-requested a review July 11, 2019 22:58
@ephiepark
Copy link
Contributor Author

ephiepark commented Jul 16, 2019

This pull request had one failure on Travis CI on (Trusty (gcc-8 + ASan + UBSan + Fuzz Test)). The cause of the failure is independent from this pull request. It's caused because gcc8 is not properly suppressing signed-integer-overflow (pointer-overflow) for ubsan. I have created an issue for it (#1686).

file(GLOB CompressSources ${LIBRARY_DIR}/compress/*.c)
file(GLOB DecompressSources ${LIBRARY_DIR}/decompress/*.c)
file(GLOB DictBuilderSources ${LIBRARY_DIR}/dictBuilder/*.c)
file(GLOB DeprecatedSources ${LIBRARY_DIR}/deprecated/*.c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ephiepark , this new GLOB is much better for file layout flexibility

@Cyan4973
Copy link
Contributor

Thanks @ephiepark ,
this patch is good to go.

As we are close to a new release, we'll wait a bit for the "freeze" snapshot, and merge this patch right after that.

@Cyan4973
Copy link
Contributor

Release completed,
we can merge.

@Cyan4973 Cyan4973 merged commit be3d2e2 into facebook:dev Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants