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

fix issue #3144 #3226

Merged
merged 2 commits into from
Aug 4, 2022
Merged

fix issue #3144 #3226

merged 2 commits into from
Aug 4, 2022

Conversation

zengyijing
Copy link
Contributor

When the value of argument long is not set, the current argument parsing logic doesn't set windowLog to the default value g_defaultMaxWindowLog and windowLog remains 0. As a result, when calling printActualCParams(), it prints out a wrong value got from ZSTD_getCParams().

@terrelln
Copy link
Contributor

terrelln commented Aug 1, 2022

I think this fix looks good, but please add a test to the tests/cli-tests/verbose-wlog.sh.

You should add a shell script to tests/cli-tests/compression that compresses a file and prints the extra verbose information. You can just write the output to -o $INTOVOID. E.g.

#!/bin/sh

set -e

. "$COMMON/platform.sh"

zstd < file -vv -19 -o file.19.zst
zstd -vv -l file.19.zst

zstd < file -vv -19 --long -o file.19.long.zst
zstd -vv -l file.19.long.zst

If you run ./tests/cli-tests/run.py --preserve, it will output the stderr to tests/cli-tests/scratch/compression/verbose-wlog/stderr. You can ask the tool to expect a particular stderr with a glob by adding a file tests/cli-tests/compression/verbose-wlog.sh.stderr.glob

...
*wlog=23*
...
Window Size: 8388608 B*
...
*wlog=27*
Window Size: 134217728 B*
...

See the README for docs on the cli-tests.

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.

See above

@zengyijing
Copy link
Contributor Author

@terrelln I've added the test as you suggested. Please take a look. Thanks!

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.

Thanks for the PR!

@terrelln terrelln merged commit d0dcc9d into dev Aug 4, 2022
@zengyijing zengyijing deleted the fix3144 branch August 4, 2022 20:53
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.

None yet

3 participants