On May 19, 2017, at 9:38 AM, Jaap Keuter <jaap.keuter@xxxxxxxxx> wrote:
> So, this change https://code.wireshark.org/review/#/c/21708/ appears to make the
> buildbot happy, the GCC6 builds still fails with
> (3346fc9c83719bce26cca31fc4b5d9139ab56d76)
>
> In file included from ./asn1/q932/packet-q932-template.c:33:0:
> ./asn1/q932/packet-q932-exp.h:26:27: error:
> ‘q932_PresentedNumberUnscreened_vals’ defined but not used
> [-Werror=unused-const-variable=]
> ./asn1/q932/packet-q932-exp.h:18:27: error: ‘q932_PresentedNumberScreened_vals’
> defined but not used [-Werror=unused-const-variable=]
> ./asn1/q932/packet-q932-exp.h:10:27: error:
> ‘q932_PresentedAddressUnscreened_vals’ defined but not used
> [-Werror=unused-const-variable=]
> ./asn1/q932/packet-q932-exp.h:2:27: error: ‘q932_PresentedAddressScreened_vals’
> defined but not used [-Werror=unused-const-variable=]
GCC 6 is probably correct.
*Defining* functions and data (except perhaps for inline functions) in a header file is generally Not The Right Thing To Do, because
1) if only one source file includes the header, and there's no reason for other files to include it, the very existence of the header is misleading, as it implies that the stuff in the header is stuff being exported from the file;
2) if more than one source file includes the header, you have multiple copies of the data or function, rather than sharing a single copy, which is arguably wasteful;
3) if more than one file includes the header, and not all of them use all of the data or functions, you have multiple copies of the data or function *and some of them are unused*, which is *definitely* wasteful.
The only reason why you *might* want to do this for data is that, if you have a plugin module, and it has a data structure that points to one of the data structures mentioned in the header, the run-time loader on some platform you support might not do the load-time evaluation of the address of the structure required to make this work. For example, if libwireshark exports a value_string table, I'm not sure all the platforms we support would allow a plugin dissector to point to that value_string from one of its header fields.
Given that we're already exporting value_string tables from libwireshark, either
1) it works on all platforms
or
2) we're not doing the stuff that doesn't work
so I wouldn't be opposed to doing with asn2wrs-generated dissectors the same way we do it for hand-written dissectors, i.e.:
have the header file declare but *not* define the value_strings, and declare it with WS_DLL_PUBLIC rather than either static or extern;
have the source files define the value strings, and define them with WS_DLL_PUBLIC_DEF.
Any value_string that asn2wrs is not *absolutely certain* will be used only by one source file should *not* be made static and should *not* be defined in a header file. Any value string that asn2wrs *is* absolutely certain will be used only by one source file should be defined as static *in that source file*, *not* in a header file.