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

make otc compilable out of the box #1111

Closed
wants to merge 4 commits into from
Closed

make otc compilable out of the box #1111

wants to merge 4 commits into from

Conversation

Source61
Copy link
Contributor

Credits to Nekiro for the commit to otland-otclient otland@c873092

@diath
Copy link
Collaborator

diath commented Sep 15, 2020

The __has_include directive is available since C++17 whereas otclient currently targets C++11, if we were going to start relying on C++17 there's a lot of things that could be reworked in the codebase.
Besides, it's the role of the build system to provide the compiler with the correct include paths as they could be literally anything.

@diath diath closed this Sep 15, 2020
@Source61
Copy link
Contributor Author

Source61 commented Sep 16, 2020

Why not rely on it? Are there any drawbacks?

Not to mention I'm confident pretty much everyone would like to see otclient reworked at this point, that's why you see projects like otclient1.0, otclientv8, otland-otclient, and otclient alternatives like OTU and Tibia Unity 3D.

@Source61
Copy link
Contributor Author

Source61 commented Sep 20, 2020

@diath
Edubart himself seem to have supported using newer C++ versions while he was still active.
"C++0x comes with a varieties of new utilities, you can and you are encouraged to use them."
Source: https://github.com/edubart/otclient/wiki/OTClient-Coding-Style/fb4e839dd7360309538ce8e8decdf97446a6ba4b (2011 -- still there in the latest revision 2015-2020)

Are there any reasons not to use newer C++ standards such as C++17?

I recognize this solution by Nekiro might be a temporary one, not entirely appropriate etc, but the solution is very small and easy to replace later on, would you really say it's better to leave otclient in a broken condition like this for months at a time?
I don't understand...

@diath
Copy link
Collaborator

diath commented Sep 20, 2020

Are there any reasons not to use newer C++ standards such as C++17?

None really other than the fact that you cannot just make a change that relies on C++17 while the project explicitly targets an older version of the standard, you have to update the CMake and Visual Studio (maybe?) projects accordingly as well as the SDK libs (if necessary, although another option would be to drop support for the SDK altogether and forget about it).

I recognize this solution by Nekiro might be a temporary one, not entirely appropriate etc, but the solution is very small and easy to replace later on, would you really say it's better to leave otclient in a broken condition like this for months at a time?

Because nothing is broken in the codebase, this is a solution to an unrelated problem (which neither the original PR nor this one describes). The problem lies within how vcpkg distributes the LuaJIT package and not how our code includes it, if you want to fix this issue then I believe the correct place to fix it is in the Visual Studio solution but I don't know much about VS to know what exactly needs to be changed.

@Source61
Copy link
Contributor Author

Source61 commented Sep 20, 2020

None really other than the fact that you cannot just make a change that relies on C++17 while the project explicitly targets an older version of the standard, you have to update the CMake and Visual Studio (maybe?) projects accordingly as well as the SDK libs (if necessary, although another option would be to drop support for the SDK altogether and forget about it).

Point by point:

  • I wasn't aware that the project targets an older C++ version. I still don't understand why one would target an older C++ version, but with that being the case, I wish such details were a little clearer/easier to find (where?).
  • I tested the fix beforehand and it worked out of the box both on Linux (cmake) and Windows (VS2019), no project changes needed.
  • Isn't the otclient SDK already so outdated that it no longer works with the client anymore anyway? I believe I tried using the SDK and got a bunch of linker errors or something (possibly misremembering?).

Because nothing is broken in the codebase, this is a solution to an unrelated problem (which neither the original PR nor this one describes). The problem lies within how vcpkg distributes the LuaJIT package and not how our code includes it, if you want to fix this issue then I believe the correct place to fix it is in the Visual Studio solution but I don't know much about VS to know what exactly needs to be changed.

That's fair. I don't either. Appreciate the answer.

@Source61
Copy link
Contributor Author

Source61 commented Oct 22, 2020

@diath
I thought a little about the fact that otclient on linux compiles using std=c++11 and __has_include works in it.
So I tested it out in a new file as well, hasinclude.cpp and compiled it with --std=c++11 successfully.
It even compiles with --std=c++03.
Whether or not it was introduced in c++17, why does it matter when it compiles for Linux using c++11 and works with Windows as well?
Sample code:
`#if __has_include("stdio.h")
#include <stdio.h>
#endif

int main()
{
printf("Test!\n");
}`

https://www.bfilipek.com/2019/07/hasinclude.html

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

2 participants