Locale fixes

[Karlson2k]

The main goal of this PR is fixing crash on Win32.
MS CRT lib is crashed when setlocale() is called from multiple threads since VS2012 (for more information see bug description and CRT source code provided with Visual Studio).
Crash can be eliminated by using _wsetlocale() instead of setlocale() or by calling _configthreadlocale(_ENABLE_PER_THREAD_LOCALE) in each thread.
To minimize chance of crash we need to use both methods, because we can't set thread local locale in externally produced threads (libmicrohttpd, Python) and some external libs use setlocale().
After enabled thread local locale, changing GUI regional settings will affect only on one thread. To overcome this AlphaNumericCompare() must use locale from g_langInfo. This will also fix bug in AlphaNumericCompare() (using reference to temporarily object).

Later we can remove changing global locale after careful testing.

[Karlson2k]

Test crash program:

#include <SDKDDKVer.h>

#include <Windows.h>
#include <locale.h>
#include <process.h>


unsigned int __stdcall threadFunc(void* p)
{
  //_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
  while (1)
  {
    setlocale(LC_NUMERIC, NULL);
    Sleep(50);
  }
  return 0;
}


int wmain(int argc, wchar_t* argv[])
{
  //_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
  for (int i = 0; i < 20; ++i)
    _beginthreadex(NULL, 0, threadFunc, NULL, 0, NULL);

  Sleep(60 * 1000);
  return 0;
}

[Memphiz]

Well out of actual hazzle i had with the GUIFontCache. Even if this is windows - please try to

1. Prevent include of boost headers in header files
2. Move implementations in the cpp files when its not a oneliner

This is about the crts_caller.h which really is a dirty header in that regard. Something like that ruined 8 hours of my free time yesterday when trying to get the GUIFontCache.h to behave with c++11 and clang. Imo its bad practice to have so much impl. in the header and the boost includes really do bad stuff which influences namespaces and what not (they forward declare something in a bad way).

I know that this will never be compiled with clang but we really have to pay attention for not polluting headers with boost includes and implementations. (call_in_all_crts in this case should be moved to the cpp for example).

[Paxxi]

This sems like a very over engineered solution for a workaround for a library bug. Why is a whole class needed when it's just opening the modules and calling one function in each? Dragging in boost seems unnecessary just to make a string of one function that we already know the name of.

I don't think using ifdefs is a reasonable way to handle indenting, just turn of indentation of namespaces in visual studio.

[Memphiz]

No really - this is what the ifdefs are about? Some IDE automation which should be worked around? No something like that should not hit any code in mainline imo.

[Karlson2k]

@Memphiz This boost include will include only two very small headers. We need stringify macro in this header and we use stringify macros in many places, so it's better to use boost version rather than define them with different names everywhere.

You can't move implementation of template function from header as linker will fail in this case. This is the reason why I removed everything that require windows header from this function and implemented it with minimal number or lines. In first implementation functions were called right after acquiring address.

This #if 0 can't hurt anyone. :) It's used in some libs (libmicrohttpd for example).

[Karlson2k]

@Paxxi It's implemented not for single function.
Same technique is used already in https://github.com/xbmc/xbmc/blob/master/xbmc/utils/Environment.cpp#L173 and I suspect we will need it for more functions. Now they can be implemented very easily.

[Karlson2k]

Updated with removed #if 0 and with commit that change JSONVariantWriter::Write() for non-win32

[Paxxi]

If we assume similar things will be needed and we use this generic approach, I'd like to see the environment code reworked to make use of this. I see no reason that we need both unless I'm missing something.

On a sidenote, it really is a horrible situation with the prebuilt libraries using whatever crt requiring this type of hacks.

[Karlson2k]

@Paxxi The reason why I didn't rewrite Environment.h/.cpp is the same why CVariant don't use any internal function: in current implementation it can be easily used outside Kodi.

Agreed that different CRTs are really bad for us. We can't even reuse file descriptors as they are valid only within one CRT lib.
But until everything will be ported to the latest VC (including libs that currently builded by MinGW), we will have no choice.

[Karlson2k]

Updated to match version merged in Helix branch.

[MartijnKaijser]

jenkins build this please

[Karlson2k]

Failed on RBPI due to jenkins error.
The rest is fine.

[Karlson2k]

Updated with very minor refactoring

jenkins build this please

[MartijnKaijser]

@Paxxi @Montellese any objections for merge?
or any other dev?

[Paxxi]

I vote no, I still think this is overengineered for what it does. Currently we have a macro that requires boost to hide a template function that creates an instance of the class just to open the the modules handles. It can all be squished into a single function and be just as flexible but less boiler plate code

[MartijnKaijser]

over engineered or not, let me remind you that windows is quite broken now. This means no alpha release and all will be delayed.

[MartijnKaijser]

so in short, something better needs to come along or it should go in regardless at the end of the month and rework it after.

[Paxxi]

Give me an hour or two to figure it out, I'll merge it myself if I don't have something better to come up with

[Karlson2k]

@Paxxi I can replace BOOST_PP_STRINGIZE with some custom macro, but stringize macros are used in many places. I'd like to replace all of them with standard one (boost).
As alternative we can add own header with stringize macros, but I think it's better to use boost version instead of homebrew.

[Paxxi]

so this is basically what I'm thinking https://gist.github.com/Paxxi/960c1836b637d5c757ea . It's exactly as flexible but less boiler plate, no boost. this was a quickie so maybe some formatting is bad/ugly

[Karlson2k]

@Paxxi It's close to my first implementation.
You missed comparing pointer to cur_fnc_ptr. And list of CRTs is initialized each call.
But I don't like such big function (with CTRs names list) in header. My implementation keep header smaller.
If the main problem is inclusion of boost header - it can be replaced with small custom macro. But we need to ensure that name of that macro will not interfere with other stringize macros. That was the reason why I decided to use small boost header. Check it - it's really small.

[Paxxi]

generally I agree about avoiding things in the header but this is only used in one single place and much of the logic was in the header anyway. It's still a small header though.

Can ofc make the list of crt names a static to avoid creating it every call.

What I mainly objected against was things like creating a wchar array to initialize a vector instead of initializing it directly. Having a class to open and store function pointers instead of calling them and closing directly and ofc the macro, don't see much point in having it and especially since the function isn't heavily used.

Also this will create fewer vectors, in your case you created and populated two vectors, one for modules and one for crts on every call to the function.

[Karlson2k]

@Paxxi Check current implementation. Only one vector is used.

[Paxxi]

well, one is static with crt names but two is being created on every call, the m_funcPtrs and m_crts

[Karlson2k]

Static one is C array.
Right, two are created, but with small content.
And main reason - <Windows.h> is not included in this header. That was another big reason, why I moved implementation to .cpp file.

[Paxxi]

Anyway, I've made my point and this discussion will probably go on a while :) I'll merge this as is so we don't delay alpha1.

[Karlson2k]

@Paxxi Nice! :)

[Montellese]

@Karlson2k: Keep in mind that we want to get rid of the boost dependency. And I'm not sure if trying to avoid to include <Windows.h> makes any sense since it's part of our pre-compiled header anyway.

[Karlson2k]

@Montellese Are we going to git rid of boost completely?? Was it decided as strategic way? I saw heavy use of boost in some recently merged PRs.
And of course we need to remove windows.h from precompiled header. It's nonsense as it's used only in small part of code, not in every file. I was planning to leave only <string> and <vector> in precompiled header. It should speedup compiling.

[Paxxi]

getting rid of boost has been one of the goals listed on trac for several years :) Not sure if it's realistic to get rid of everything but at least try and avoid it as much as possible

[MartijnKaijser]

The PRs that got merged shouldn't of had boost in them. It caused @Memphiz sleepless night to get that sorted out again with the C++11 merge because it broke OSX/iOS

[Karlson2k]

This PR will not break anything, but I'll publish new PR with boost replace to make everyone happy. :)