clang build is broken by -Wdocumentation flag

[phord]

make clangbuild fails on clang-6 and clang-10 because the -Wdocumentation flag produces numerous warnings in several header files.

I bisected this to 9f8b180d5daebe54b33e0f6e02a5186dd578f838, but I'm not sure what is an appropriate fix for the current code.

make clangbuild
Cleaning completed
clang -v
clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final)
...
CC obj/conf_88500d5ea7a349d2e3a0a24ace4e5e23/static/pool.o
In file included from .//common/pool.c:15:
In file included from .//common/zstd_internal.h:28:
.//common/../zstd.h:2437:4: error: '@result' command used in a comment that is not attached to a function or method declaration [-Werror,-Wdocumentation]
  @result : 0 : successful decoding, the `ZSTD_frameHeader` structure is correctly filled.
  ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.//common/../zstd.h:2438:66: error: '@result' command used in a comment that is not attached to a function or method declaration [-Werror,-Wdocumentation]
           >0 : `srcSize` is too small, please provide at least @result bytes on next attempt.
                                                                ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.//common/../zstd.h:2457:14: error: '@return' command used in a comment that is not attached to a function or method declaration [-Werror,-Wdocumentation]
  which can @return an error code if required value is too large for current system (in 32-bits mode).
            ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.//common/../zstd.h:2477:3: error: '@result' command used in a comment that is not attached to a function or method declaration [-Werror,-Wdocumentation]
 @result of ZSTD_decompressContinue() is the number of bytes regenerated within 'dst' (necessarily <= dstCapacity).
 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 errors generated.

[phord]

Some of the breakage comes from earlier commits than the one I bisected to. I was bisection-testing only on zstd.h, but there are also warnings in dictBuilder/cover.h and other header files.

[phord]

make clangbuild works (clang-10) if I remove -Wdocumentation and add -Wno-implicit-int-float-conversion.

[eli-schwartz]

I previously noted this at https://github.com/facebook/zstd/blob/caf2fa170b42157860592c26747c7a1be5e1b045/build/meson/meson.build#L19-L21

Pretty sure no one really runs that target...

[eli-schwartz]

There are actually more warnings with clang than just that. Disabling Wdocumentation and Wimplicit-int-float-conversion, there are still these meson issues:

$ ninja -C builddir
ninja: Entering directory `builddir'
[68/94] Compiling C object tests/longmatch.p/.._.._.._tests_longmatch.c.o
../../../tests/longmatch.c:58:8: warning: 'ZSTD_initCStream_advanced' is deprecated: use ZSTD_CCtx_reset, see zstd.h for detailed instructions [-Wdeprecated-declarations]
  rc = ZSTD_initCStream_advanced(ctx, NULL, 0, params, 0);
       ^
../../../lib/zstd.h:2246:1: note: 'ZSTD_initCStream_advanced' has been explicitly marked deprecated here
ZSTD_DEPRECATED("use ZSTD_CCtx_reset, see zstd.h for detailed instructions")
^
../../../lib/zstd.h:1100:72: note: expanded from macro 'ZSTD_DEPRECATED'
#    define ZSTD_DEPRECATED(message) ZSTDLIB_STATIC_API __attribute__((deprecated(message)))
                                                                       ^
1 warning generated.
[81/94] Compiling C++ object contrib/pzstd/pzstd.p/.._.._.._.._contrib_pzstd_Options.cpp.o
../../../contrib/pzstd/Options.cpp: In member function ‘std::string pzstd::Options::getOutputFile(const string&) const’:
../../../contrib/pzstd/Options.cpp:414:37: warning: conversion from ‘std::__cxx11::basic_string<char>::size_type’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
  414 |     int stemSize = inputFile.size() - kZstdExtension.size();
      |                    ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
[82/94] Compiling C++ object contrib/pzstd/pzstd.p/.._.._.._.._contrib_pzstd_Pzstd.cpp.o
../../../contrib/pzstd/Pzstd.cpp: In function ‘uint64_t pzstd::writeFile(pzstd::SharedState&, pzstd::WorkQueue<std::shared_ptr<pzstd::BufferWorkQueue> >&, FILE*, bool)’:
../../../contrib/pzstd/Pzstd.cpp:590:37: warning: conversion from ‘std::size_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
  590 |       SkippableFrame frame(out->size());
      |                            ~~~~~~~~~^~
[84/94] Compiling C++ object contrib/gen_html/gen_html.p/.._.._.._.._contrib_gen_html_gen_html.cpp.o
../../../contrib/gen_html/gen_html.cpp:128:29: warning: "/*" within comment [-Wcomment]
  128 |         /* comments of type /**< and /*!< are detected and only function declaration is highlighted (bold) */
      |                              
../../../contrib/gen_html/gen_html.cpp:128:38: warning: "/*" within comment [-Wcomment]
../../../contrib/gen_html/gen_html.cpp:162:29: warning: "/*" within comment [-Wcomment]
  162 |         /* comments of type /*! mean: this is a function declaration; switch comments with declarations */
      |                              
../../../contrib/gen_html/gen_html.cpp:182:57: warning: "/*" within comment [-Wcomment]
  182 |         } else if (exclam == '=') { /* comments of type /*= and /**= mean: use a <H3> header and show also all functions until first empty line */
      |                                                          
../../../contrib/gen_html/gen_html.cpp:182:65: warning: "/*" within comment [-Wcomment]
../../../contrib/gen_html/gen_html.cpp:194:38: warning: "/*" within comment [-Wcomment]
  194 |         } else { /* comments of type /** and /*- mean: this is a comment; use a <H2> header for the first line */
      |                                       
../../../contrib/gen_html/gen_html.cpp:194:46: warning: "/*" within comment [-Wcomment]
[94/94] Linking target contrib/pzstd/pzstd

It is mostly contrib/, but there is one deprecated declaration used in a test binary.

[terrelln]

It is mostly contrib/, but there is one deprecated declaration used in a test binary.

@eli-schwartz This is allowed, and the test/ directory should ignore deprecation errors with -Wno-deprecated-declarations.

I'm fixing everything else & adding CI.

[eli-schwartz]

@eli-schwartz This is allowed, and the test/ directory should ignore deprecation errors with -Wno-deprecated-declarations.

This isn't a generally applicable rule -- many people instead argue that tests should not ignore deprecation errors unless the tests themselves are testing that the deprecated interface still works.

Obviously opinions vary...

[terrelln]

This isn't a generally applicable rule -- many people instead argue that tests should not ignore deprecation errors unless the tests themselves are testing that the deprecated interface still works.

Yeah, the tests use the deprecated interfaces only to test them.

[eli-schwartz]

Yeah, the tests use the deprecated interfaces only to test them.

The function that triggers the warning I posted above doesn't seem to be in code exclusively testing that, it's just in setup code -- furthermore, that function is used by two other test/ files, but does not trigger warnings in those because those files say:

#define ZSTD_DISABLE_DEPRECATE_WARNINGS /* No deprecation warnings, we still test some deprecated functions */

So I'm inclined to think that this test is not, in fact, using the deprecated interface only to test it. :)

[terrelln]

Good point, I missed the filename. That one can definitely be updated!