Display command line parameters with concrete values in verbose mode

[ghost]

Continue addressing #2834

[ghost]

Ah, it seems that some of the tests are failing due to using different CFLAGS than those that are present in the cmake build, I will address these.

[Cyan4973]

I believe the intention of #2834 is to know the parameters used in "default" circumstances, when they are not specified on command line.

For example :

xz -kf -vv FILE
xz: Filter chain: --lzma2=dict=8MiB,lc=3,lp=0,pb=2,mode=normal,nice=64,mf=bt4,depth=0
xz: 94 MiB of memory is required. The limiter is disabled.
xz: Decompression will need 9 MiB of memory.

Note how xz is not provided any compression parameter on command line,
yet the -vv verbose command is providing detailed compression parameters in use.

The intention is to reproduce this level of details,
and then, possibly, extend it with additional capabilities that zstd supports but xz doesn't.
But we'll have to look at this second part, and pay attention to information clutter :
we don't want to use precious console space to display mass amount of details of negligible importance.

For reference, currently, using this PR, I'm getting :

./zstd -kf -vv FILE
*** zstd command line interface 64-bits v1.5.1, by Yann Collet ***
--format=.zst --no-sparse --block-size=0 --memory=134217728 --threads=1 --content-size
Decompression will require 39607 B of memory

The intention would rather be to generate a line like (for example):

--zstd=wlog=23,clog=23,hlog=22,slog=6,mml=3,tlen=48,strat=6

and then possibly extend it with additional options if they are useful.

[ghost]

So more or less what --show-default-cparams is presently doing, but in a format that is directly compatible with the command line args?

[Cyan4973]

So more or less what --show-default-cparams is presently doing, but in a format that is directly compatible with the command line args?

Yes, pretty close.

One expected difference though is that these parameters should be the ones actually used by the compressor.
So it's not "just" the "default" parameters, but if any of these parameters end up being adjusted,
this should be reflected on screen.

[Cyan4973]

From a high level perspective, this PR does the expected job.
Now, a few tactical considerations :

• The display logic is split into 2 parts, between fileio.c/FIO_displayCompressionParameters(), and zstdcli.c/main().
  That does not seem a good idea. Consider:
  • move any display logic out of main() into its own function.
    • For consistency, this should be applied to showDefaultCParams too, even if it's not your fault that its display logic is currently within main()
  • For which reason is displayCompressionParameters() positioned within fileio.c ? In absence of any specific reason, it seems it should rather be present within zstdcli.c, since fileio.c is more about functions related to opening / reading / writing files and pipes.
• Could you imagine a way to test this new feature ? Not necessarily a complex / exhaustive test, but at least something that can check the basics, able to trigger an error signal if, in some future, the feature is erroneously cancelled by some future contribution.

[ghost]

The reason displayCompressionParameters() is in fileio.c is that from zstdcli.c, FIO_prefs_t is an opaque struct (i.e. an incomplete type) and thus its fields cannot be accessed. I could remedy this by exposing the struct definition in the header file, but it looks like it is deliberately typedef'd right now to maintain an abstraction barrier, so I assumed you'd want to keep it that way.

[Cyan4973]

The reason displayCompressionParameters() is in fileio.c is that from zstdcli.c, FIO_prefs_t is an opaque struct (i.e. an incomplete type) and thus its fields cannot be accessed. I could remedy this by exposing the struct definition in the header file, but it looks like it is deliberately typedef'd right now to maintain an abstraction barrier, so I assumed you'd want to keep it that way.

Yeah, these are good points.

With both angles having pros and cons, there is no "obvious" better solution.
So okay, let's keep it that way, with displayCompressionParameters() remaining within fileio.c.

[ghost]

@Cyan4973 I noticed some GitHub actions are failing due to size_t not matching printf formatting directives because it is defined differently on different platforms. Normally I'd resolve this by using the %zu directive, but that was introduced with C99, and I remember you stating that the codebase adheres to C90, so I'm not sure how you want to address this.

[Cyan4973]

Indeed, %zu would be the correct directive, but unfortunately is not C90 compliant.

We generally solve this situation by using %u instead.
This, in turn, requires casting the value to display with (unsigned)val.

[Cyan4973]

Great PR @Svetlitski-FB !
I believe it's in good shape, and only have one minor request :

Could you merge your test, in tests/test_verbose_output.sh
as part of test/playTests.sh ?
It seems to be a fast enough test, so it can be played as part of the baseline test suite which is always triggered (make check), typically near https://github.com/facebook/zstd/blob/dev/tests/playTests.sh#L769 to remain on-topic.

[Cyan4973]

Thanks for delivering this new feature @Svetlitski-FB !