Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visual Studio 2010 support #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Visual Studio 2010 support #15

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 19, 2015

This adds support for building tinygettext static libraries with Visual Studio. It is still WIP.
On my to-do list:

  • Add support for building static libraries
  • Add support for building dynamic libraries

@Grumbel
Copy link
Member

Grumbel commented Dec 19, 2015

Some issues with this patch:

  • src/win32/ is missing from the patch, but referenced in CMakeLists.txt
  • replacing explicit operator bool() with non-explicit version is a bad idea, should use operator void*()
  • there is some Win32 stuff over in this fork https://github.com/SuperTux/tinygettext that I haven't yet had a look at
  • some of the #ifdef _WIN32 stuff maybe should be MSVC specific, as I thing MinGW works fine with the way it is (I guess, haven't really compiled it myself in ages)

Also a general question: Is MSVC2010 support and thus the reduction of C++11 features necessary? Aren't there newer Microsoft compiler that have better C++11 support?

@leper
Copy link
Member

leper commented Dec 19, 2015

Since VS2013 (and even 2015 IIRC) support building for XP, but don't run on it, and are both (in the Express Version) available free of charge I don't really see the point in supporting whatever subset of C++11 VS2010 supports. (Even VS2013 does not have full support, but everything used in tinygettext currently is supported.)

You should also note that since the API of tinygettext uses e.g. std::string a library (be that static or dynamic) has to be compiled with the same version of the compiler (and the same settings) as otherwise you might end up with type mismatches. So you'd have to add a wrapper to take either char* or provide custom types that are only allocated on one side of the program<->lib barrier.

Moreover the way to include tinygettext would be just pure inclusion in your source tree, given that you probably want to use already existing file loading logic.

@rmarquis
Copy link

rmarquis commented Jan 9, 2016

some of the #ifdef _WIN32 stuff maybe should be MSVC specific, as I thing MinGW works fine with the way it is (I guess, haven't really compiled it myself in ages)

This is correct. I tested with Mingw-w64 and it compiles without issue. The _WIN32 definitions should be changed to MSVC specific definitions.

@leper
Copy link
Member

leper commented Jan 10, 2016

Am I assuming correctly (after reading [1]) that you need a fix for the missing <dirent.h> for Windows? In that case [2] might be of interest.

EDIT: It seems like you've already worked around that in a way that seems quite nice, want to submit that as a pull request?

[1] https://dev.etlegacy.com/issues/914
[2] leper@9f98bf2

@rmarquis
Copy link

Yes, in any case we need some workaround for MSVC. We were using the dirent.h implementation trick when using the old tinygettext code, but for now we're using the native "windows_file_system" which I find cleaner. This is directly taken from this pull request, so kudo to @sparklerfox for his work!

We're close to a new ET:Legacy release, so at the moment we quickly integrated the patches we needed downstream. In the future, we'd like to closely follow upstream implementation with your CMake system. Both dirent.h or windows_file_system additions work, so the choice is yours here.

The real issue here would be the c++11 code. We statically compile our official releases against the oldest glibc available (on CentOS) for maximum compatibility across Linux distribution, so for now we can't really use these c++11 features due to old GCC, and we reverted to the old code (see etlegacy/etlegacy@5832d1d).

Would a patch that allows usage of both c++11 or the old c++ standard through a macro be accepted upstream? Something like #ifdef CPP11 ... #else ... #endif.

@leper
Copy link
Member

leper commented Jan 14, 2016

Ah, that was hidden in a commit with lots of other not really related changes. Guess we'll have to do some cherry picking since adding a Windows implementation for that seems nicer (and shorter) than having to provide a dirent.h implementation

What version of CentOS and GCC (and libstdc++) is that? The current code is supported by at least 4.6+.

I'd rather not include a patch adding pointless ifdefs, reverting to pre-C++11 also doesn't seem that nice. (I guess I'll still not merge some of my wip stuff for simplifying a few things by using things C++ provides given that you don't support that yet)

@rmarquis
Copy link

Our official builds are compiled on CentOS 6.x, which provides GCC 4.4.7 (and I guess libstdc++.so.6). I do understand that you'd rather avoid including ugly ifdefs, so I guess we'll keep that patch downstream for the time being.

Another possibility would be to maintain an "old gcc" version in a separate branch, so downstream projects like ours could help maintain it in the official repository. This would also allow you to do whatever you want in the master branch.

@leper
Copy link
Member

leper commented Jan 15, 2016

The only thing that 4.4 supports that you removed in that one commit above is deleted functions.

I guess keeping that commit downstream or in a branch of yours shouldn't be an issue. You can then rebase that branch on top of master and use that when updating the included version. I'd rather not have an "official" branch since that gives some expectation of support for that.

Reverting to pre-C++11 doesn't seem that nice since at least some of that old code results in warnings (which would break users that compile with -Werror) when compiling with -std=c++0x or -std=c++11. However I don't plan on adding more C++11 code that breaks your branch.

@rmarquis
Copy link

Fair enough. Feel free to add c++ code if you wish to do so. As long as we maintain our own downstream "pre-c++11" patch, this wouldn't make any difference anyway.

I'll check that deleted function again. For some reason mingw-w64 wouldn't let me compile on windows with gcc-4.9.x (without -std=c++11).

@leper
Copy link
Member

leper commented Jan 15, 2016

I still want to keep the difference (and the amount of work you have to do when updating) small. And there aren't a lot of changes that have to be done since the library is mostly stable anyway.

I guess it only works if you specify the standard, so for 4.4 you'd need -std=c++0x.
In case you want to keep the behaviour you could add something like the following instead:

private:
    Dictionary(const Dictionary&);
    Dictionary& operator=(const Dictionary&);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants