LordAro wrote:

Wasn't this 'unfixed' in r23640? http://hg.openttd.org/openttd/trunk.hg/rev/aa20e2f1c847

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment11718

NG wrote:

That's the one.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment11719

Alberth wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment11720

NG wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment11721

NG wrote:

Perhaps change the ticket to "Stop using __forceinline"?

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment11812

NG wrote:

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.

Attachments

• msvc2012_fix.diff (0.80 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment12071

NG wrote:

Well, looks like I pushed the developer silencing button...

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment12109

Alberth wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment12155

NG wrote:

Better this way?

Attachments

• vs2012fix.diff (1.11 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment12156

adf88 wrote:

There is a macro for these purposes (disabling the <xkeychk.h> check): _ALLOW_KEYWORD_MACROS

Attachments

• fix-vs2012-allow-macroizing-keywords.diff (0.62 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment12430

LordAro wrote:

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

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment14555

adf88 wrote:

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).

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375#comment14578

andythenorth closed the ticket.

Reason for closing: Won't implement

Flyspray clean up, issue has aged, there is no sign that it's still relevant to anyone, closing. Thanks.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5375