Bug 105460

Are you using gcc or clang or the MSVC C compiler?

I think gcc will warn about ignoring the result of functions decorated with _DBUS_GNUC_WARN_UNUSED_RESULT without needing extra compiler flags. See dbus/dbus-macros.h: the definition of the macro is guarded by a __GNUC__ check. That's why it has GNUC in the name.

clang probably will too, because I think clang pretends to be gcc (it defines __GNUC__).

Other compilers, including MSVC, won't. If you know how to make MSVC warn about an unused result, please say (and also tell the GLib developers, because G_GNUC_WARN_UNUSED_RESULT is basically the same macro).

If there's no way to do this portably, it seems better to have only gcc warn about something than to have no compilers at all warn about it (and we get gcc warnings on Travis-CI, even if you're building with MSVC yourself).

One day I'd like to have a CI build of dbus for Windows done with MSVC on Appveyor, similar to the way Travis-CI tests cross-compiling from Linux for Windows with gcc, but I haven't had time for a while to learn how to do that.

Sorry, I forget to mention the compiler - it is i686-w64-mingw32 version 7.2  (In reply to Simon McVittie from comment #1)
> Are you using gcc or clang or the MSVC C compiler?
gcc i686-mingw32 version 7.2. 
 
> I think gcc will warn about ignoring the result of functions decorated with
> _DBUS_GNUC_WARN_UNUSED_RESULT without needing extra compiler flags. See
> dbus/dbus-macros.h: the definition of the macro is guarded by a __GNUC__
> check. That's why it has GNUC in the name.
rechecked on a different host and now got the warning - seemed to be a build system not recompiling the patched file

> clang probably will too, because I think clang pretends to be gcc (it
> defines __GNUC__).
yes, checked - /home/xxx/src/dbus-1/dbus/dbus-sysdeps-unix.c:1392:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
      _dbus_socket_get_invalid ();
      ^~~~~~~~~~~~~~~~~~~~~~~~

> Other compilers, including MSVC, won't. If you know how to make MSVC warn
> about an unused result, please say (and also tell the GLib developers,
> because G_GNUC_WARN_UNUSED_RESULT is basically the same macro).

https://stackoverflow.com/questions/4226308/msvc-equivalent-of-attribute-warn-unused-result
mentions _check_return_, which requires to set compile switch /analyze.

refers to https://msdn.microsoft.com/en-us/library/jj159529(v=vs.110).aspx mentions to use '_Must_inspect_result_' 
 
> If there's no way to do this portably, it seems better to have only gcc warn
> about something than to have no compilers at all warn about it (and we get
> gcc warnings on Travis-CI, even if you're building with MSVC yourself).
yes, I also do development with cross compiled gcc

I think there are two ways this bug could go.

If you mostly develop with gcc anyway, we could rely on gcc for all our -Wunused-result needs, ignore MSVC, and close this as NOTABUG.

Or, we could use the MSVC annotations:

(In reply to Ralf Habacker from comment #2)
> https://stackoverflow.com/questions/4226308/msvc-equivalent-of-attribute-
> warn-unused-result
> mentions _check_return_, which requires to set compile switch /analyze.
> 
> refers to https://msdn.microsoft.com/en-us/library/jj159529(v=vs.110).aspx
> mentions to use '_Must_inspect_result_' 

I don't think we can *require* MSVC users to use /analyze, but if these attributes are accepted and ignored by the non-analyzing MSVC compiler, then using them would be fine.

If you want to make use of that annotation, because its positioning seems to be more strict than gcc attributes, I think the way to do it would be:

* add a new _DBUS_WARN_UNUSED_RESULT which expands to
  _Must_inspect_result_ on (recent) MSVC, __attribute__((warn_unused_result))
  on gcc and compatible compilers, and nothing on other compilers

* give that macro a doc-comment that indicates how to use it correctly,
  in terms that a non-MSVC-user like me will understand
  (if I'm reading the docs correctly, it needs to go on both declaration
  and definition, before the static storage class if the function is static,
  and before the return type if not static; if there is a required order
  for whether __declspec-based macros appear before or after these
  annotations then you'll also need to document that)

* change all uses of _DBUS_GNUC_WARN_UNUSED_RESULT to
  _DBUS_WARN_UNUSED_RESULT, and when each one is changed, move the
  annotation to the right place(s) where MSVC will accept it

* remove _DBUS_GNUC_WARN_UNUSED_RESULT when it has no more users

Created attachment 138067 [details] [review]
patch proposal (requires msvc >= 2012)

(In reply to Ralf Habacker from comment #4)
> Created attachment 138067 [details] [review] [review]
> patch proposal (requires msvc >= 2012)

A colleague is currently testing this patch on a machine with msvc 2012 and will provide a working patch.

Created attachment 138071 [details] [review]
enable msvc warning

After researching the Internet, the annotation should be used in
declaration and implementation. After own tests with the MSVC 2012
compiler it is sufficient to use the annotation only in the
declaration to get a compiler warning, as with the GCC compiler.
So the annotation is not necessary in the C implementation.

Comment on attachment 138071 [details] [review]
enable msvc warning

Review of attachment 138071 [details] [review]:
-----------------------------------------------------------------

This looks very promising. Would it be possible to contribute a similar macro to GLib, while you're looking at this? In the GLib case, G_GNUC_WARN_UNUSED_RESULT would have to stay as-is for backwards compat (since it's public API, unlike _DBUS_WARN_UNUSED_RESULT), but setting up for better warnings on all compilers seems like a very good thing.

Are there really no other uses of _DBUS_GNUC_WARN_UNUSED_RESULT? I'm surprised there aren't more...

We should definitely add it in more places (like all the _dbus_string_foo() functions that return FALSE on out-of-memory) but I'll delay doing that until this commit is ready, to avoid adding a lot of _DBUS_GNUC_WARN_UNUSED_RESULT and then immediately having to turn it into _DBUS_WARN_UNUSED_RESULT.

::: cmake/CMakeLists.txt
@@ +218,5 @@
>      # 4114 same type qualifier used more than once
>      # 4133 'type' : incompatible types - from 'type1' to 'type2'
>      set(WARNINGS_ERRORS "4002 4003 4013 4028 4031 4047 4114 4133")
> +    if(MSVC_VERSION GREATER 1600 AND ${CMAKE_BUILD_TYPE} STREQUAL "Debug")
> +        set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /analyze")

How "expensive" is /analyze?

If it's something that makes sense to use all the time for your debug builds (like gcc -Og -g) then you might as well go ahead with this. If it's really slow, then we'd probably be better off leaving out this change, and developers who really want static analysis can opt-in to it by setting whatever is CMake's equivalent of CFLAGS.

::: dbus/dbus-macros.h
@@ +92,4 @@
>  #define DBUS_ALLOC_SIZE2(x,y)
>  #endif
>  
> +/* MSVC requires to specify this warning in the declaration */

Please expand this comment to tell me *where* in the declaration (bear in mind that Linux developers like me have to be able to get this right in future code contributions, without ever actually using MSVC themselves). gcc attributes can be used like

__attribute__((foo)) static void foo (bar);
static void foo (bar) __attribute__((foo));

or even (I think)

static void __attribute__((foo)) foo (bar);
static __attribute__((foo)) void foo (bar);

but from what I've seen in documentation, MSVC attributes like _Must_inspect_result_ must come before the return type?

Is there any requirement about putting _DBUS_WARN_UNUSED_RESULT before or after the "static" of a static declaration?

Is there any requirement about putting _DBUS_WARN_UNUSED_RESULT before or after a macro that expands to __declspec(something), such as like DBUS_PRIVATE_EXPORT?

(In reply to Ralf Habacker from comment #4)
> patch proposal (requires msvc >= 2012)

If it would make your life easier to put a Visual Studio requirement in NEWS, perhaps something like

* Visual Studio >= 20xx is required when compiling with CMake and MSVC

then I'd have no problem with that. (It would be best if at least one of the free Community Editions is supported, of course.)

(In reply to Simon McVittie from comment #7)
> Comment on attachment 138071 [details] [review] [review]
> enable msvc warning
> 
> Review of attachment 138071 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Are there really no other uses of _DBUS_GNUC_WARN_UNUSED_RESULT? I'm
> surprised there aren't more...

The macro is still used in the test program test/test-utils.h. I correct this in the following patch.


> ::: cmake/CMakeLists.txt
> @@ +218,5 @@
> >      # 4114 same type qualifier used more than once
> >      # 4133 'type' : incompatible types - from 'type1' to 'type2'
> >      set(WARNINGS_ERRORS "4002 4003 4013 4028 4031 4047 4114 4133")
> > +    if(MSVC_VERSION GREATER 1600 AND ${CMAKE_BUILD_TYPE} STREQUAL "Debug")
> > +        set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /analyze")
> 
> How "expensive" is /analyze?
> 
> If it's something that makes sense to use all the time for your debug builds
> (like gcc -Og -g) then you might as well go ahead with this. If it's really
> slow, then we'd probably be better off leaving out this change, and
> developers who really want static analysis can opt-in to it by setting
> whatever is CMake's equivalent of CFLAGS.

With an Intel Core i7-2640M, the following times are required for compilation:

With /analyze 0:03:22.945000
Without /analyze 0:01:43.620000

Without the option /analyze, it is about 50 percent faster. So, it's really expensive, I guess. In the next patch is a CMake option that disables this by default.

> 
> ::: dbus/dbus-macros.h
> @@ +92,4 @@
> >  #define DBUS_ALLOC_SIZE2(x,y)
> >  #endif
> >  
> > +/* MSVC requires to specify this warning in the declaration */
> 
> Please expand this comment to tell me *where* in the declaration (bear in
> mind that Linux developers like me have to be able to get this right in
> future code contributions, without ever actually using MSVC themselves). gcc
> attributes can be used like
> 
> __attribute__((foo)) static void foo (bar);
> static void foo (bar) __attribute__((foo));
> 
> or even (I think)
> 
> static void __attribute__((foo)) foo (bar);
> static __attribute__((foo)) void foo (bar);
> 
> but from what I've seen in documentation, MSVC attributes like
> _Must_inspect_result_ must come before the return type?
> 
> Is there any requirement about putting _DBUS_WARN_UNUSED_RESULT before or
> after the "static" of a static declaration?
> 
> Is there any requirement about putting _DBUS_WARN_UNUSED_RESULT before or
> after a macro that expands to __declspec(something), such as like
> DBUS_PRIVATE_EXPORT?

I did some tests and expanded the comment. In this context I found the usage of this macro in dbus-userdb.h, where the macro is misplaced for the msvc compiler. But this has no effect because the file is only used under Linux. That is why we could leave it at that in this case.

Created attachment 138099 [details] [review]
Enable "unused result" warning for MSVC Compiler

Enable "unused result" warning for MSVC Compiler (Visual Studio >= 2012, MSVC 11.0)

(In reply to Daniel Wendt from comment #9)
> In this context I found the usage
> of this macro in dbus-userdb.h, where the macro is misplaced for the msvc
> compiler. But this has no effect because the file is only used under Linux.
> That is why we could leave it at that in this case.

If this macro is going to have a MSVC implementation, then I think I'd prefer to adopt "put this macro where MSVC needs it to be" as part of our coding style, to make the rule to follow simpler for reviewers.

Comment on attachment 138099 [details] [review]
Enable "unused result" warning for MSVC Compiler

Review of attachment 138099 [details] [review]:
-----------------------------------------------------------------

::: cmake/CMakeLists.txt
@@ +176,4 @@
>      ADD_DEFINITIONS(-D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE)
>      SET(CMAKE_C_FLAGS_DEBUG   "${CMAKE_C_FLAGS_DEBUG}   /FIconfig.h")
>      SET(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} /FIconfig.h")
> +    option (DBUS_MSVC_CODEANALYZE "Enable code analyzing for MSVC compiler: /analyze" OFF)

Maybe DBUS_MSVC_ANALYZE? Then we don't have to argue about whether CODEANALYZE is one word or two :-)

@@ +219,4 @@
>      # 4114 same type qualifier used more than once
>      # 4133 'type' : incompatible types - from 'type1' to 'type2'
>      set(WARNINGS_ERRORS "4002 4003 4013 4028 4031 4047 4114 4133")
> +    if(DBUS_MSVC_CODEANALYZE AND MSVC_VERSION GREATER 1600 AND ${CMAKE_BUILD_TYPE} STREQUAL "Debug")

I don't see much point in conditioning this on CMAKE_BUILD_TYPE==Debug any more, if it's off by default anyway.

::: dbus/dbus-macros.h
@@ +105,5 @@
> + * static inline returnvalue _DBUS_WARN_UNUSED_RESULT functionName();
> + *
> + * does not work with MSVC, produces compiling errors:
> + * DBUS_PRIVATE_EXPORT returnvalue functionName _DBUS_WARN_UNUSED_RESULT ();
> + * DBUS_PRIVATE_EXPORT returnvalue functionName() _DBUS_WARN_UNUSED_RESULT;

Thanks for testing all these. I think this could be summarized as something like:

"""
This macro is used in function declarations. Unlike gcc-specific attributes, to avoid compilation failure with MSVC it must appear somewhere before the function name in the declaration. Our preferred coding style is to place it before the return type, for example

DBUS_PRIVATE_EXPORT _DBUS_WARN_UNUSED_RESULT
dbus_bool_t _dbus_user_database_lock_system (void);
"""

That way, we are documenting both the requirement (must be somewhere before the function name) and the coding-style recommendation (should be before the return type).

@@ +123,5 @@
>  /** @def _DBUS_GNUC_NORETURN
>   * used to tell gcc about functions that never return, such as _dbus_abort()
>   */
> +/** @def _DBUS_WARN_UNUSED_RESULT
> + * used to tell gcc and msvc about functions whose result must be used

Please combine this doc-comment with the comment above, something like this:

/** @def _DBUS_WARN_UNUSED_RESULT
 *
 * An attribute for functions whose result must be checked by the caller.
 *
 * This macro is used in function declarations. [etc.]
 */
#if defined(_MSC_VER) && ...
#define ...
#elif ... [etc.]

::: dbus/dbus-userdb.h
@@ +94,4 @@
>  DBUS_PRIVATE_EXPORT
>  DBusUserDatabase* _dbus_user_database_get_system    (void);
>  DBUS_PRIVATE_EXPORT
> +dbus_bool_t       _dbus_user_database_lock_system   (void) _DBUS_WARN_UNUSED_RESULT;

To be consistent with portable and Windows-specific code, and to avoid reviewers having to think about whether they are looking at portable, Windows-specific or Unix-specific code as much as possible, please move _DBUS_WARN_UNUSED_RESULT onto the line with DBUS_PRIVATE_EXPORT.

Created attachment 138126 [details] [review]
Enable "unused result" warning for MSVC Compiler

patch update

I also updated the Readme.cmake

Comment on attachment 138126 [details] [review]
Enable "unused result" warning for MSVC Compiler

Review of attachment 138126 [details] [review]:
-----------------------------------------------------------------

Looks good to me!

::: README.cmake
@@ +166,5 @@
>  DBUS_BUILD_X11:BOOL=ON
>  
> +MSVC only (Visual Studio >= 2012):
> +// Enable code analyzing for MSVC compiler: /analyze
> +DBUS_MSVC_ANALYZE:BOOL=OFF

Thanks, I always forget to update this file :-)

Comment on attachment 138126 [details] [review]
Enable "unused result" warning for MSVC Compiler

Review of attachment 138126 [details] [review]:
-----------------------------------------------------------------

What version of dbus did you base this patch on? It doesn't seem to apply cleanly to git master with `git am`.

New feature changes should be done on the master branch. Older branches like dbus-1.12 and dbus-1.10 are bugfix-only.

I'll attach a version that's been rebased on master for someone who has MSVC to test.

Created attachment 138129 [details] [review]
Enable "unused result" warning for Visual Studio >= 2012 (MSVC 11.0)

From: Daniel Wendt

The _Must_inspect_result_ annotation is documented to be used in both
the declaration and implementation, but in testing with the MSVC 2012
compiler it appears to be sufficient to use the annotation only in the
declaration to get a compiler warning, as with the GCC compiler.
So the annotation is not necessary in the C implementation.
                                                                         
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=105460
[smcv: Rebase dbus-sysdeps.h changes on master]
[smcv: Clarify commit message]
Reviewed-by: Simon McVittie <smcv@collabora.com>

---

Someone with MSVC should test this. Also available on my Github repository: https://github.com/smcv/dbus/commits/must-inspect-result-105460

CI in progress with various gcc-based configurations: https://travis-ci.org/smcv/dbus/builds/353801680

One day I'd like to have similar CI for CMake+MSVC+Windows in AppVeyor or similar, but I never finished working out how to get suitable versions of libexpat and GLib installed on an AppVeyor CI worker, so we don't have that. appveyor.yml contributions welcome!

(In reply to Simon McVittie from comment #15)
> Comment on attachment 138126 [details] [review] [review]
> Enable "unused result" warning for MSVC Compiler
> 
> Review of attachment 138126 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> What version of dbus did you base this patch on? It doesn't seem to apply
> cleanly to git master with `git am`.
> 
> New feature changes should be done on the master branch. Older branches like
> dbus-1.12 and dbus-1.10 are bugfix-only.
> 
> I'll attach a version that's been rebased on master for someone who has MSVC
> to test.

Sorry, that was my fault.

I just tested the rebased patch. It works with and without the /analyze option :-)

(In reply to Simon McVittie from comment #16)
> One day I'd like to have similar CI for CMake+MSVC+Windows in AppVeyor or
> similar

Bug #105521

Fixed in git for 1.13.4