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

Moved stdtr1compat to a header file #349

Merged

Conversation

monkey0506
Copy link
Contributor

Also checks for VS2015, which is the earliest version of Visual Studio which doesn't need the TR1 namespace for these headers (AFAICT).

@sonneveld
Copy link
Member

sonneveld commented Aug 15, 2016

Is this actually tidying it up or is this just adding complication via macros? Could we leave it messier for now given that all this would go away if we force everyone to use a recent compiler? I believe some copy/pasta is fine until there's a good abstraction.

@monkey0506
Copy link
Contributor Author

I would argue that this header removes complications and makes the code more readable. I also don't believe there are any plans to move away from VS2008 as the default Windows compiler in the near future, unless that has changed due to @ivan-mogilko stripping the pre-compiled libs. In any case, I think keeping the option open of compiling without C++11 as a strict enforcement is preferable for some (and Microsoft has refused thus far to implement the entire C++11 standard). Other files may also wish to include similar headers before the project files are migrated to a newer Visual Studio, and I think it would be best to not redefine "stdtr1compat" in every single file that needs it.

@monkey0506
Copy link
Contributor Author

Furthermore, something of this nature is required if someone wants to use VS2015 to build, as the __cplusplus macro does not report C++11 (due to missing core features from the standard), so the existing conditions are incorrect for VS2015.

@sonneveld
Copy link
Member

sonneveld commented Aug 15, 2016

Hrmm, fair enough. I wonder if we could combine your work with this stackoverflow answer: http://stackoverflow.com/a/5956576/84262 (specifically TR1INCLUDE) or just replace the whole thing with boost.TR1 :) I'm just worried that we'll end up with some leaky abstraction but I guess that can't be avoided.

@monkey0506
Copy link
Contributor Author

monkey0506 commented Aug 15, 2016

@sonneveld How's this? Note that this also invalidates #348 by pulling in the missing header.

@sonneveld
Copy link
Member

That does look pretty tidy! Looks like linux and android builds are failing. ios/osx seem to work (they just fail when merged with latest master)

@monkey0506
Copy link
Contributor Author

Looks like I forgot to include the AGS_NEEDS_TR1 macro in that branch of the condition. Pushing a fix now.

@ghost
Copy link

ghost commented Aug 15, 2016

Note that this also invalidates #348 by pulling in the missing header.

To clarify, we may just close #348 if #349 is accepted?
I think this pull is okay, this is something that had to be done. So if you are certain about this request, you may close (cancel) #348 yourself.

@ghost
Copy link

ghost commented Aug 15, 2016

I think I know why do you need these changes right now, are you probably using some kind of custom branch that has #346 and master merged in, as well as MSVS 2015 solution.

While these changes are good on their own, there is a different problem with them that I can foresee: at the moment we do not have MSVS 2015 built in the build server (nor actively used by developers for this project), which means that there will be no automatic checks made for repository branches. If anyone includes these headers "old way" again, or do anything else not-2015-compatible, this will break 2015th build once more and they won't notice. This will also require a person who has MSVS 2015 to periodically check if master builds for him/her and possibly append fixes.

To fully close this issue we would need to properly support MSVS 2015 solution in the repository and have server build it as a dedicated task (along with MSVS 2008 or instead - this is something to consider).

@ghost
Copy link

ghost commented Aug 15, 2016

Of course what I said above is not a criticism of the proposed code change on its own, but rather the warning if you assumed master to be compatible with MSVS 2015 from now on.

@monkey0506
Copy link
Contributor Author

I definitely understand that this isn't a final solution to building with VS2015, but I still see that as tangential to this PR. Ideally we shouldn't accept any (new) use of TR1 headers that aren't passed through this compat layer or else other existing target platforms will likely fail to build anyway.

@sonneveld
Copy link
Member

I'm happy if VS2015 support breaks every now and again until we add VS2015 to the build server. Hopefully the breakages will be infrequent if at all.

@ivan-mogilko ivan-mogilko merged commit 0d08230 into adventuregamestudio:master Aug 16, 2016
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