OpenTTD

Tasklist

FS#5375 - Fix __forceinline in Visual Studio 2012

Attached to Project: OpenTTD
Opened by NG (NG) - Monday, 26 November 2012, 22:04 GMT
Last edited by andythenorth (andythenorth) - Saturday, 02 September 2017, 12:17 GMT
Type Patch
Category Build system
Status Closed
Assigned To No-one
Operating System Windows
Severity Medium
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Implements the suggestion from http://www.tt-forums.net/viewtopic.php?f=33&t=61266, tested with 64-bit Windows 8 Ultimate.
This task depends upon

Closed by  andythenorth (andythenorth)
Saturday, 02 September 2017, 12:17 GMT
Reason for closing:  Won't implement
Additional comments about closing:  Flyspray clean up, issue has aged, there is no sign that it's still relevant to anyone, closing. Thanks.
Comment by Charles Pigott (LordAro) - Wednesday, 28 November 2012, 11:39 GMT Comment by NG (NG) - Thursday, 29 November 2012, 08:45 GMT
That's the one.
Comment by Alberth (Alberth) - Friday, 30 November 2012, 14:35 GMT
First of all, why should we revert the change?


Secondly, the patch seems to bluntly replace 'inline' with 'INLINE'. Did you verify whether each routine is actually useful to inline? I would be very amazed if that would actually be the case.

Thirdly, if you add forcing to inline a function, how will you manage that for all platforms that we have (ie the general form would be "inline this function for that and that platform, not for this one, and I don't care for the other ones"). Your patch does not seem to support that notion, nor do I see a sane way to do that, nor would I know how to decide which function to inline when.

I trust compilers more than ourselves in deciding what to inline, especially with the wide range of platforms that are supported.
Comment by NG (NG) - Friday, 30 November 2012, 18:18 GMT
I didn't say anything specifically about reverting, just that the problem starts after that revision, and certainly didn't look at each individual function.
If all you'd like is to let the compiler make the decisions in every case, only that one "#define inline __forceinline" needs to be taken out.
Comment by NG (NG) - Wednesday, 26 December 2012, 23:48 GMT
Perhaps change the ticket to "Stop using __forceinline"?
Comment by NG (NG) - Saturday, 02 March 2013, 13:39 GMT
So once again, if the "#define inline __forceinline" bits are taken out of squirrel.h and stdafx.h, there are no errors or warnings at all. This also seems to be what you wanted all along.
The only thing I haven't tested is DirectMusic, which is entirely unnecessary anyway.
Comment by NG (NG) - Saturday, 23 March 2013, 03:44 GMT
Well, looks like I pushed the developer silencing button...
Comment by Alberth (Alberth) - Sunday, 14 April 2013, 09:09 GMT
Somewhat. Most devs (including me) don't use windows, which makes it very awkward to do anything with such patches.

I wonder what happens with the window builds that do not use VS 2012. That line was not added to the source code for nothing.

To be on the safe side, I guess your removal should only be done when you use the new VS version.
Comment by NG (NG) - Sunday, 14 April 2013, 17:28 GMT
Better this way?
Comment by Grzegorz Duczyński (adf88) - Sunday, 21 July 2013, 06:50 GMT
There is a macro for these purposes (disabling the <xkeychk.h> check): _ALLOW_KEYWORD_MACROS
Comment by Charles Pigott (LordAro) - Friday, 18 August 2017, 22:52 GMT
I'd recommend going with the "Stop using __forceinline" option

Especially since it still doesn't guarantee the function being inlined ( https://msdn.microsoft.com/en-us/library/bw1hbe6y.aspx ) and the compiler generally knows better than you (tm) anyway
Comment by Grzegorz Duczyński (adf88) - Saturday, 19 August 2017, 11:03 GMT
I'm not saying that I'm against removing __forceinline, but it's not a problem that it doesn't guarantee actual code inlining. __forceinline says "don't perform cost analysis, create an in-line definition only if possible".

The 'inline' keyword, except of its language meaning, sometimes affects the compiler cost analysis on code in-lining e.g. see http://stackoverflow.com/questions/27042935/inline-keyword-vs-inlining-concept. The macro makes MSVC more similar to them.

Programmers tend to use 'inline' to say "try harder at code inlining". You may be of course against this practice (abusing 'inline' keyword) but it's already in motion anyway.

The __forceinline might have to do also with the MSVC code generation options - maybe it's to compensate the "optimize for code size" option (not sure).

Loading...