Maratyszcza / pthreadpool

Portable (POSIX/Windows/Emscripten) thread pool for C/C++
BSD 2-Clause "Simplified" License
342 stars 132 forks source link

Move deprecated attribute to the end of declarations #5

Closed mwtarnowski closed 4 years ago

mwtarnowski commented 4 years ago

Including pthreadpool.h in a source file causes g++ to emit several warnings during compilation, like the following:

In file included from main.cc:2:
/home/pthreadpool/include/pthreadpool.h:185:14: warning: ‘pthreadpool_function_1d_t’ is deprecated [-Wdeprecated-declarations]
  185 |  size_t range);
      |              ^
In file included from main.cc:2:
/home/pthreadpool/include/pthreadpool.h:174:39: note: declared here
  174 | typedef PTHREADPOOL_DEPRECATED void (*pthreadpool_function_1d_t)(void*, size_t);
      |                                       ^~~~~~~~~~~~~~~~~~~~~~~~~

For some reason g++ cannot recognize that usage of a deprecated type occurs in a function that is marked as deprecated itself. However, when the attribute deprecated is placed at the end of a declaration (it is an order suggested by the GCC specification for both functions and types) everything seems ok - the compiler no longer complains.

Look also at the snippet in Compiler Explorer.

Maratyszcza commented 4 years ago

The "postfix" style is incompatible with MSVC. Do you actually need the deprecated functions? If no, you can specify -DPTHREADPOOL_ALLOW_DEPRECATED_API=OFF in CMake.

mwtarnowski commented 4 years ago

No, I don't need the deprecated functions. I compile my project with -Werror flag and I just find it surprising that including pthreadpool.h crashes the compilation. By adding the line #include <pthreadpool.h> I am forced to define also PTHREADPOOL_NO_DEPRECATED_API.

The changes shouldn't affect MSVC - currently PTHREADPOOL_DEPRECATED macro is empty when __GNUC__ is missing. Do you plan using __declspec(deprecated) for MSVC?

Alternatively, consider disabling the deprecated functions by default and set a flag to enable legacy api for projects that need it.

Maratyszcza commented 4 years ago

Yes, I plan to support Windows (and MSVC). However, as the current version probably doesn't compile with MSVC anyway, I figure its ok to merge now, and make it build with MSVC later.