After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 742851 - avoid MSVC warnings in G_STMT_END
avoid MSVC warnings in G_STMT_END
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2015-01-13 13:09 UTC by Paolo Borelli
Modified: 2015-01-14 15:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (1.20 KB, patch)
2015-01-13 13:09 UTC, Paolo Borelli
committed Details | Review
use G_STMT_START/END in gslice (1.40 KB, patch)
2015-01-13 13:12 UTC, Paolo Borelli
committed Details | Review
use G_STMT_START/END in gtestutils (8.30 KB, patch)
2015-01-13 13:13 UTC, Paolo Borelli
committed Details | Review

Description Paolo Borelli 2015-01-13 13:09:20 UTC
MSVC does not like "while (0)", the attached patch conditionally shuts up the warning in the macro definition.

As mentioned in the commit message the trick is borrowed from http://cnicholson.net/2009/03/stupid-c-tricks-dowhile0-and-c4127/

I tested building my project on MSVC 2010 and it works fine.
Comment 1 Paolo Borelli 2015-01-13 13:09:55 UTC
Created attachment 294423 [details] [review]
patch
Comment 2 Paolo Borelli 2015-01-13 13:12:36 UTC
Created attachment 294424 [details] [review]
use G_STMT_START/END in gslice

Trivial follow up patch that makes use of the macro instead of open coding do{}while(0)
Comment 3 Paolo Borelli 2015-01-13 13:13:09 UTC
Created attachment 294425 [details] [review]
use G_STMT_START/END in gtestutils

Trivial follow up patch that makes use of the macro instead of open coding do{}while(0)
Comment 4 Fan, Chun-wei 2015-01-13 15:20:24 UTC
Hi Paolo,

It seems like this stems from one building GLib-using code and/or GLib using /W4, which is just below the highest warning level, /Wall (/W4 reports all the warnings sans the ones here[1]), with all possible warnings from Visual Studio turned on.  This is why I don't normally see this when building GLib or GLib-using items with Visual Studio.

You can take a look at the warning level in the project under "Project Properties", in the C/C++ section, under general.

For the Visual Studio project files we have in GNOME, the warning level that is normally set is /W3, which is the normal default-I think it is likely that there will be also other warnings when one uses /W4 (there could be many warnings, multi-platform libraries often hit this, by the way) that we would probably deem to be annoying, but may be useful to catch potential problems in the code during developement.

I'm not totally sure whether we really want to do these though, as a result, sorry.

p.s. For references, another way to achieve the same effect, would be to update the msvc_recommended_pragmas.h and add something like #pragma warning(disable:4127)	and add a comment for that, and make sure that the project force-includes msvc_recommended_pragmas.h to achieve the same effect.  This header should be included with every GLib dev files shipped for Windows though.

Hope this explains the situation.

With blessings!

[1]: http://msdn.microsoft.com/en-us/library/23k5d385.aspx
Comment 5 Fan, Chun-wei 2015-01-13 15:29:11 UTC
Hi Paolo,

...oh, by the way, for the latter 2 patches, I think they are okay to me if it also works for you on the other platforms as well, the previous comment was solely regarding disabling the C4127 part.

With blessings, thank you!
Comment 6 Paolo Borelli 2015-01-13 15:53:11 UTC
Hi Fan!

thanks for the reply.

Yes, this is with /W4.

I think it is useful to have this patch since this is not only about compiling Glib/Gtk itself, but also for building other projects that use glib.h, for instance I am trying to use /M4 in my own project and this patch already removes a lot of false positives

> p.s. For references, another way to achieve the same effect, would be to update
> the msvc_recommended_pragmas.h and add something like #pragma
> warning(disable:4127)    and add a comment for that, and make sure that the
> project force-includes msvc_recommended_pragmas.h to achieve the same effect. 
> This header should be included with every GLib dev files shipped for Windows
> though.

I do not like this approach: my patch only disable that warning for that specific use of whil(0) not in general for all the sources.

If you look on the web it seems a pretty common approach to define multiline macros on MSVC and to be honest I do not see any downside in applying the patch.
Comment 7 Fan, Chun-wei 2015-01-14 09:03:38 UTC
Hello Paolo,

> ... for instance I am trying to use /M4 in my own project and this patch 
> already removes a lot of false positives

> I do not like this approach: my patch only disable that warning for that
> specific use of whil(0) not in general for all the sources.

I see what you are getting at... (I thought you wanted to say bye to the C4127 warnings as a whole), then I think it is fine with me when dealing with the other /W4 warnings are fine on your part, as a result.  Please go ahead in getting the patches in then.

With blessings.
Comment 8 Allison Karlitskaya (desrt) 2015-01-14 14:01:00 UTC
Review of attachment 294423 [details] [review]:

Okay after two minor changes.

Commit message says nothing about 'G_STMT_END' and it should.

::: glib/gmacros.h
@@ +280,3 @@
  * This intentionally does not use compiler extensions like GCC's '({...})' to
  * avoid portability issue or side effects when compiled with different compilers.
+ * On MSVC we shut up the annoying C4127: “Conditional expression is constant”

I'd prefer that this was less confrontationally wordded.
Comment 9 Allison Karlitskaya (desrt) 2015-01-14 14:01:41 UTC
Review of attachment 294424 [details] [review]:

Sure.  Thanks.
Comment 10 Allison Karlitskaya (desrt) 2015-01-14 14:03:15 UTC
Review of attachment 294425 [details] [review]:

Looks fine.


The BEGIN { (void) 0; } END thing looks hilariously pointless, but it's not hurting anyone, either...
Comment 11 Paolo Borelli 2015-01-14 15:26:59 UTC
Reworded both the commit message and the comment and pushed. Thanks!