Add printf format attribute

[strk]

Fixes warnings on downstream projects with -Wsuggest-attribute=format

[mwtoews]

Re: MSVC errors, the Internet tells me about Microsoft source-code annotation language (SAL), described in this Q/A.

Some example implementations from GDAL and Folly. So (somewhat tested) something like:

#ifdef _MSC_VER
#include <sal.h>
typedef void (*GEOSMessageHandler)(_Printf_format_string_ const char *fmt, ...);
#else
typedef void (*GEOSMessageHandler)(const char *fmt, ...) __attribute__ (( format(printf, 1, 0) ));
#endif

Should -Wsuggest-attribute=format be added to CMakeLists.txt?

[strk]

@mwtoews good idea about adding the -WSuggest-attribute=format in CMakeLists.txt will see about doing that.

For the record, the downstream suggestion/failures can be seen here:
https://woodie.osgeo.org/repos/30/pipeline/2187/7#L907

[strk]

For the record, PostGIS uses this:

#ifndef __GNUC__
# define __attribute__(x)
#endif

[pramsey]

CI builds with warnings-as-errors, so now you need to actually follow up all the warnings :)

[strk]

For now I've removed the -Wsuggest-attribute=format from the Makefile as I hadn't understood which line enables it only for the compiler supporting the attribute syntax.

[strk]

So, MingW suggests the gnu_printf attribute but I don't know if it supports passing attribute

../capi/geos_ts_c.cpp:301:31: error: function 'void GEOSContextHandle_HS::NOTICE_MESSAGE(const char*, ...)' might be a candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format]

[strk]

The Windows / mingw-w64 keeps suggesting the attribute even if we're now defining it. I surrender. I don't even know why I'm wasting my time trying to make window builds happy

[mwtoews]

@strk should I take a look at the msvc/mingw aspects? I can plop some commits here if needed.

[strk]

@strk should I take a look at the msvc/mingw aspects? I can plop some commits here if needed.

Yes please. If you can push directly go ahead, otherwise send a PR against this new branch.

[mwtoews]

The mingw issue is a mystery (why it want's to add attributes to built-ins), and I'm able to replicate this locally. This oddity is side-stepped to ignore the warning within __MINGW32__ defs.

The MSVC printf format stuff is handled differently, so it is probably easiest to put some portability macros into include/geos/export.h (I'd rather not create a separate port.h for this minor thing). MSVC's sal.h/_Printf_format_string_ method only triggers warnings with the -analize flag, which I've tested locally to work. A later enhancement could enable this, but is out-of-scope for this PR.

Additional review / commits welcome!

[strk]

include/geos/export.h is not included in geos_c.h so they should be replicated there for clients

[mwtoews]

include/geos/export.h is not included in geos_c.h

Looks fine to me:

geos/capi/geos_c.h.in

Line 86 in effd6cb

#include <geos/export.h>

And I've just checked that these files are installed to the prefix, e.g. /usr/local/include/geos/export.h

[strk]

Oh then it's perfect for me, I'll just go ahead and merge this. It probably needs a NEWS entry too at some point

[pramsey]

This change actually added warnings for me...

In file included from rt_band.c:37:
In file included from ./librtcore_internal.h:35:
In file included from ../../liblwgeom/lwgeom_geos.h:31:
/usr/local/include/geos_c.h:105:16: warning: redefinition of typedef 'GEOSMessageHandler' is a C11 feature [-Wtypedef-redefinition]
typedef void (*GEOSMessageHandler)(const char *fmt, ...);

../../liblwgeom/lwgeom_geos.h:28:16: note: previous definition is here
typedef void (*GEOSMessageHandler)(const char *fmt, ...) __attribute__ (( format(printf, 1, 0) ));

[pramsey]

Considering changing the macro to this:

#if POSTGIS_GEOS_VERSION < 31300
#if __STDC_VERSION__ >= 201112L
/* See https://github.com/libgeos/geos/pull/1097 */
/* Only redefine the typedef if compiler is C11 or higher */
typedef void (*GEOSMessageHandler)(const char *fmt, ...) __attribute__ (( format(printf, 1, 0) ));
#endif
#endif

[rouault]

and why not simply just not defining GEOSMessageHandler in lwgeom_geos.h, since it is available in geos_c.h ... ?

[pramsey]

@strk can explain I guess he wants that extra attribute when he's compiling against older GEOS versions that don't have the attribute in the library include typedef. (Looks dodgy to me too.)

[strk]

and why not simply just not defining GEOSMessageHandler in lwgeom_geos.h, since it is available in geos_c.h ... ?

Because until GEOS 3.13 the typedef of GEOSMessageHandler was not passing the printf attribute thus resulting in a warning about "why not add an attribute printf ?"

Where did you get 201112L from ?
I've committed that macro change in https://git.osgeo.org/gitea/postgis/postgis/commit/e3529a98caef2a0018fea994feff2c15e270ea1b

[pramsey]

Where did you get 201112L from ?

ChatGPT 🫢

[pramsey]

Well, that doesn't work because in CI we have C declaring itself to be 1999 and yet still also complaining about the attribute warning. Why don't we just turn off that warning for all GEOS < 3.13? Fixing GEOS warnings in PostGIS by redefining GEOS types feels off to me.

lwgeom_geos_relatematch.c:56:23: error: argument 2 of 'initGEOS' might be a candidate for a format attribute [-Werror=suggest-attribute=format]
   56 |  initGEOS(lwpgnotice, lwgeom_geos_error);
      |                       ^~~~~~~~~~~~~~~~~

Something like this in the configure.ac? So far works for me quieting builds on the GH CI.

+dnl Turn off attribute warnings for GEOS < 3.13
+if test "$POSTGIS_GEOS_VERSION" -lt "31300"; then
+       _LT_COMPILER_OPTION([if $compiler supports -Wno-suggest-attribute=format], [_cv_nosuggestattribute], [-Wno-suggest-attribute=format], [], [CFLAGS="$CFLAGS -Wno-suggest-attribute=format"], [])
+fi

[strk]

May I suggest to move this discussion to the PostGIS issue tracker instead ?