Fix warnings #1281

mathstuf · 2019-01-11T18:43:00Z

These are warnings that show up when building VTK's trimmed down netcdf tree. It is by no means "all" warnings.

DennisHeimbigner

It is always good to kill warning, but we have generally not worried
about unused parameters. The strncat code properly needs to be converted
to strlcat, which is now our preferred method. For the parameters,
I would prefer using unnamed parameters instead of the (void)param
approach.

mathstuf · 2019-01-11T19:15:27Z

For the parameters, I would prefer using unnamed parameters instead of the (void)param
approach.

I was getting compiler errors about unnamed parameters with that approach (which is what I tried first).

DennisHeimbigner · 2019-01-11T19:20:00Z

Interesting. What compiler and what was the error?

mathstuf · 2019-01-11T19:40:54Z

GCC 8.2.1:

/usr/lib64/ccache/cc -DENABLE_SET_LOG_LEVEL -DH5_BUILT_AS_DYNAMIC_LIB -DHAVE_CONFIG_H -DRELAX_COORD_BOUND -I/home/boeckb/code/depot/group-kitware/third-party/netcdf/src/include -I/home/boeckb/code/depot/group-kitware/third-party/netcdf/src/oc2 -I/home/boeckb/code/depot/group-kitware/third-party/netcdf/src/libsrc -I. -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -g -Wall -Wconversion -fPIC -MD -MT libdispatch/CMakeFiles/dispatch.dir/nclog.c.o -MF libdispatch/CMakeFiles/dispatch.dir/nclog.c.o.d -o libdispatch/CMakeFiles/dispatch.dir/nclog.c.o   -c /home/boeckb/code/depot/group-kitware/third-party/netcdf/src/libdispatch/nclog.c
/home/boeckb/code/depot/group-kitware/third-party/netcdf/src/libdispatch/nclog.c: In function ‘nclogtextn’:                                                                                           
/home/boeckb/code/depot/group-kitware/third-party/netcdf/src/libdispatch/nclog.c:213:12: error: parameter name omitted                                                                                
 nclogtextn(int /*tag*/, const char* text, size_t count)
            ^~~

mathstuf · 2019-01-11T19:41:39Z

C99 apparently doesn't allow it: https://stackoverflow.com/questions/8776810/parameter-name-omitted-c-vs-c

DennisHeimbigner · 2019-01-11T19:58:25Z

Ugh!

t-b · 2019-01-14T09:58:12Z

Can we have a macro for making the unused warning go away? Something like

#define NC_UNUSED(A) do{(void) (A);}while(0)

with that nobody needs to wonder what the purpose of (void) A; is.

mathstuf · 2019-01-14T19:05:53Z

@t-b Sure. What header should I add it to?

DennisHeimbigner · 2019-01-14T19:33:00Z

I think the right place to put that macro would be in include/ncconfigure.h

This should be used to indicate that a variable is unused in a codeblock.

This could overflow if `tmp` only had a few bytes left in its space given its current contents.

It ends up being `basetime_1 + basetime_2` with a space in between. Make enough room to ensure that it is not truncated.

mathstuf · 2019-01-16T20:56:07Z

Updated to add NC_UNUSED to ncconfigure.h and use it.

t-b · 2019-01-16T20:59:41Z

@mathstuf I tend to always use the do/while hack in macros but I'm not the one making the decisions here ;)

mathstuf · 2019-01-16T21:01:29Z

Ah, I usually only use it for multi-line expansions or those which otherwise don't allow for a ; to be in the calling code. I'm fine with rebasing again if do/while(0) is preferred.

DennisHeimbigner · 2019-01-16T21:04:41Z

Is there any obvious situation where (void)var will not work?

mathstuf · 2019-01-16T21:15:07Z

I can't think of any off-hand. Searching via Github or using Debian Code Search is quite noisy, so I can't point to many other projects doing it. Qt seems to think it sufficient: https://github.com/qt/qtbase/blob/5.12/src/corelib/global/qglobal.h#L117

edhartnett · 2019-02-02T14:51:07Z

libdispatch/dparallel.c

@@ -102,6 +102,11 @@ int nc_create_par(const char *path, int cmode, MPI_Comm comm,
                  MPI_Info info, int *ncidp)
 {
 #ifndef USE_PARALLEL
+    NC_UNUSED(path);


I disagree with this. This clutters up the code only in order to suppress warnings? That's what a warning suppression file is for.

These warnings appear from VTK's embedding of NetCDF into its source tree (mainly for Windows users' convenience) where these warnings appear with our default CI configurations. We can certainly keep the patches in our import, but I'm trying to minimize our diffs against upstream code.

As for clutter, I don't see this as "clutter". It's an explicit acknowledgement that path is not used in the non-parallel path. I wish there were a way to say "warn if this line is no longer necessary", but I don't know of any. This line also fixes the warning for all known compilers with any given set of warning flags. Doing per-compiler suppression regexes or pragmas is, IME, way more complicated.

I can also go back to the literal (void)path; code rather than adding a new (internal) macro. Would adding a // unused parameter comment on the lines help?

edhartnett · 2019-02-02T14:55:59Z

The usual procedure on projects I've worked on is that the developers build configuration should build without warnings, not that all configurations should build without warnings.

I would support fixing any warnings for other configurations which can be fixed without adding complexity to the code base (i.e. lines of code). In many cases this can be accomplished, in others not.

Where not, for those who delight (as I do) in warning-free builds, the answer is the warning suppression file. This can be used to turn off specific code warnings that are known to be unimportant, and allow new warnings to pop out as they should.

But I don't agree that we should add macros to the code solely to support warning free builds in configurations. That is adding code to support something that's not a requirement.

DennisHeimbigner · 2019-02-02T19:35:47Z

developers build configuration
I was not aware of this configuration; of what ./configure flags does it consist?

without adding complexity to the code base (i.e. lines of code).
I agree; this PR technically violates this goal

the warning suppression file.
I assume the use of this is compiler dependent.

For gcc, we have the option to use the -Wno-unused-parameter compiler option
to suppress these warnings. Clang also supports a similar flag. For visual studio, it
looks like we would need to add #pragme statements. Other compilers
unknown.

mathstuf · 2019-02-04T13:56:01Z

For gcc, we have the option to use the -Wno-unused-parameter compiler option
to suppress these warnings. Clang also supports a similar flag. For visual studio, it
looks like we would need to add #pragme statements. Other compilers
unknown.

Maybe I'm missing something, but this sounds like a lot more than one line change per unused parameter. It certainly doesn't sound better according to this metric IMO:

I would support fixing any warnings for other configurations which can be fixed without adding complexity to the code base (i.e. lines of code). In many cases this can be accomplished, in others not.

edhartnett · 2019-02-04T16:50:04Z

@DennisHeimbigner when I say developers build configuration, I mean the one the developers use, i.e. non-paralllel, GNU tools based, on a linux-like system.

Right now the C library builds with -Wall with only two warnings. If we would remove those two, we could have a CI job that enforced this warning free build, and that would be great.

However, to build without warnings on any arbitrary platform and compiler is too high of a bar.

gsjaardema · 2019-02-04T16:52:53Z

I like the NC_UNUSED(var) macro solution and started using in some of my C libraries. It is IME much clearer than the (void)var approach and is simple to use. Also very easy to grep the source code if you ever need to find the instances.

I definitely support the build without warnings practice.

DennisHeimbigner · 2019-02-04T19:18:14Z

Maybe I'm missing something, but this sounds like a lot more than one line change per unused parameter.

Not sure I follow. If you are referring to the compiler options, then that is a change
to configure.ac and does not modify any code at all.

mathstuf · 2019-02-04T19:45:03Z

How is configure.ac not code? Does nothing in that file have a maintenance burden on NetCDF?

edhartnett · 2019-02-04T19:54:57Z

Yes configure.ac is code but no changes are required to support compiler flags. Simply set them before calling configure, that's how it works.

What I have seen done on other projects is to identify a canonical developers build on the CI system, and turn on the compiler option which causes that build to fail if there are warnings. In that way, no one can check in code that contains warnings without failing CI.

However, I have not seen at attempt made to make warning-free builds everywhere. I am in support of such efforts which do not cause lines of code to be added. I am not in support if they do.

For those who want warning-free builds on other platforms, where that cannot be achieved without needlessly adding to code complexity, I would suggest warning suppression files be used locally to suppress warnings that are annoying.

MetalKnight · 2020-01-02T21:44:24Z

Hello,
just to let you know that the latest NetCDF-C and NetCDF-C++ have the following warning that is preventing linking against installed shared object library and exported include files.

netcdf4/include/ncGroup.h:18:14: error: ‘netCDF::file_id’ defined but not used [-Werror=unused-variable]
static int file_id;
^
cc1plus: all warnings being treated as errors

mathstuf · 2020-01-06T15:44:52Z

Thanks, I recommend filing a new issue or PR for that. I only fixed the issues I found in the release of netcdf we were using at the time.

MetalKnight · 2020-01-06T15:57:11Z

@mathstuf thanks, I've opened a new issue in the C++4 project Unidata/netcdf-cxx4#85

mathstuf requested review from DennisHeimbigner and WardF as code owners January 11, 2019 18:43

DennisHeimbigner reviewed Jan 11, 2019

View reviewed changes

mathstuf force-pushed the fix-warnings branch from 7d6c026 to 520584a Compare January 14, 2019 19:06

mathstuf added 7 commits January 16, 2019 15:51

ncconfigure: add NC_UNUSED macro

eda449b

This should be used to indicate that a variable is unused in a codeblock.

warnings: mark conditionally used parameters

a747216

ncuri: check the right preprocessor definition

eb0b9b8

warnings: mark unused parameters

b3f3443

NC_mktmp: pass the remaining space to strncat

94a0259

This could overflow if `tmp` only had a few bytes left in its space given its current contents.

warnings: store sizes in size_t

8c33122

nctime: expand basetime's size

82d72df

It ends up being `basetime_1 + basetime_2` with a space in between. Make enough room to ensure that it is not truncated.

mathstuf force-pushed the fix-warnings branch from 520584a to 82d72df Compare January 16, 2019 20:55

WardF added 2 commits January 23, 2019 15:12

Merge branch 'master' into fix-warnings

688c06d

Merge branch 'master' into fix-warnings

9ca942b

edhartnett reviewed Feb 2, 2019

View reviewed changes

WardF mentioned this pull request Feb 6, 2019

Rinse and Repeat: PR Edition #1309

Merged

WardF merged commit 9ca942b into Unidata:master Feb 7, 2019

mathstuf deleted the fix-warnings branch February 7, 2019 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix warnings #1281

Fix warnings #1281

mathstuf commented Jan 11, 2019

DennisHeimbigner left a comment

mathstuf commented Jan 11, 2019

DennisHeimbigner commented Jan 11, 2019

mathstuf commented Jan 11, 2019

mathstuf commented Jan 11, 2019

DennisHeimbigner commented Jan 11, 2019

t-b commented Jan 14, 2019 •

edited

Loading

mathstuf commented Jan 14, 2019

DennisHeimbigner commented Jan 14, 2019

mathstuf commented Jan 16, 2019

t-b commented Jan 16, 2019

mathstuf commented Jan 16, 2019

DennisHeimbigner commented Jan 16, 2019

mathstuf commented Jan 16, 2019

edhartnett Feb 2, 2019

mathstuf Feb 4, 2019

edhartnett commented Feb 2, 2019

DennisHeimbigner commented Feb 2, 2019 •

edited

Loading

mathstuf commented Feb 4, 2019

edhartnett commented Feb 4, 2019

gsjaardema commented Feb 4, 2019

DennisHeimbigner commented Feb 4, 2019 •

edited

Loading

mathstuf commented Feb 4, 2019

edhartnett commented Feb 4, 2019

MetalKnight commented Jan 2, 2020

mathstuf commented Jan 6, 2020

MetalKnight commented Jan 6, 2020

Fix warnings #1281

Fix warnings #1281

Conversation

mathstuf commented Jan 11, 2019

DennisHeimbigner left a comment

Choose a reason for hiding this comment

mathstuf commented Jan 11, 2019

DennisHeimbigner commented Jan 11, 2019

mathstuf commented Jan 11, 2019

mathstuf commented Jan 11, 2019

DennisHeimbigner commented Jan 11, 2019

t-b commented Jan 14, 2019 • edited Loading

mathstuf commented Jan 14, 2019

DennisHeimbigner commented Jan 14, 2019

mathstuf commented Jan 16, 2019

t-b commented Jan 16, 2019

mathstuf commented Jan 16, 2019

DennisHeimbigner commented Jan 16, 2019

mathstuf commented Jan 16, 2019

edhartnett Feb 2, 2019

Choose a reason for hiding this comment

mathstuf Feb 4, 2019

Choose a reason for hiding this comment

edhartnett commented Feb 2, 2019

DennisHeimbigner commented Feb 2, 2019 • edited Loading

mathstuf commented Feb 4, 2019

edhartnett commented Feb 4, 2019

gsjaardema commented Feb 4, 2019

DennisHeimbigner commented Feb 4, 2019 • edited Loading

mathstuf commented Feb 4, 2019

edhartnett commented Feb 4, 2019

MetalKnight commented Jan 2, 2020

mathstuf commented Jan 6, 2020

MetalKnight commented Jan 6, 2020

t-b commented Jan 14, 2019 •

edited

Loading

DennisHeimbigner commented Feb 2, 2019 •

edited

Loading

DennisHeimbigner commented Feb 4, 2019 •

edited

Loading