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

Fix for clang compilation #299

Closed
wants to merge 13 commits into from
Closed

Fix for clang compilation #299

wants to merge 13 commits into from

Conversation

q4a
Copy link
Member

@q4a q4a commented Dec 16, 2018

Some time ago clang compilation was broken, so I fixed it.
Travis: added to 4 clang related configs (clang-6/clang-7 on x64 with Release/Debug target).
Travis: commented out 2 configs: gcc-7 Debug target on x64/x86 - it was building more 50 mins and was always terminated by travis. Debug target building with clang now and we will know if somebody will broke it.

@@ -151,4 +151,4 @@ class CProblemSolver
virtual void clear();
};

#include "xrAICore/Components/problem_solver_inline.h"
//#include "xrAICore/Components/problem_solver_inline.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Check full commit - I moved it:
q4a@710cd10

Copy link
Contributor

@FreeZoneMods FreeZoneMods Dec 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the headers with the '_inline' suffix in the name are usually included to the headers without the suffix (and only to this headers). Moving the include could mislead somebody.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got some problems, when moved xrAICore from xrGame. One of them here - problem_solver_inline.h does not know about "ai()":
https://travis-ci.org/q4a/xray-16/jobs/462642365#L1952
When I added
#include "xrGame/ai_space.h"
I got another error:
xrAICore/Components/problem_solver_inline.h:367:36: error: member access into incomplete type 'CGraphEngine'
m_failed = !ai().graph_engine().search(*this, reverse_search ? target_state() : current_state(),
^
/home/1/code/xray-16/src/xrAICore/../xrAICore/AISpaceBase.hpp:8:7: note: forward declaration of 'CGraphEngine'

So CGraphEngine is main problem: it includes "xrAICore/Components/problem_solver.h" (L23) before adding template for "search" (L104):

#include "xrAICore/Components/problem_solver.h"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FreeZoneMods we should check this commit:
Zegeri@578e942
may be it will help to aviod removing problem_solver_inline.h from problem_solver.h

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should find a way to remove ai() call from this problem solver at all? Calling ai() itself seems strange here.
And seems like it's just a warning, not an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think about removing ai() from here, but I'm already checked Zegeri's commit:
Zegeri@578e942
He fixed problem of ai(), but not fixed access into incomplete type 'CGraphEngine' - here is clang build log:
https://travis-ci.org/q4a/xray-16/jobs/469356988#L7898

class text_tree;
}

class CBaseMonster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only for debug mode?

Copy link
Member Author

@q4a q4a Dec 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because problem was only in debug mode ( here is travis build status before this fix: https://travis-ci.org/q4a/xray-16/builds/463837730 ).
And it's based on "base_monster.h". It's only for debug mode in base_monster.h too:

@@ -1535,7 +1535,8 @@ void game_cl_mp::generate_file_name(string_path& file_name, LPCSTR file_suffix,
#else
void game_cl_mp::generate_file_name(string_path& file_name, LPCSTR file_suffix, time_t& date_time)
{
xr_sprintf(file_name, "%s_%s", ctime(date_time), file_suffix);
std::time_t std_date_time = date_time;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this temporary variable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I write "ctime(&(std::time_t)date_time)" - I'll get error:
src/xrGame/game_cl_mp.cpp:1539:42: error: cannot take the
address of an rvalue of type 'std::time_t' (aka 'long')
xr_sprintf(file_name, "%s_%s", ctime(&(std::time_t)std_date_time)...
So I based on this answers:
https://stackoverflow.com/questions/36576285/convert-long-int-to-const-time-t

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the date_time variable is a reference to a type 'time_t' (I suggest you to mark is additionaly as const). So does it mean that 'std::time_t' and 'time_t' are different types here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, different types here.
https://travis-ci.org/OpenXRay/xray-16/jobs/468604698#L3347 - gcc warning says "invalid conversion from ‘time_t {aka long int}’ to ‘const time_t* {aka const long int*}’ [-fpermissive]"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The types are not so different, but the solution looks ugly. And ifdef's, of course... I reimplement the function in PR #301, please check if the warning disappear.

@Xottab-DUTY Xottab-DUTY added this to the Linux port milestone Dec 16, 2018
@Saiv46
Copy link

Saiv46 commented Jul 16, 2019

Any news?

This was linked to issues Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Broken compilation with LLVM 8.0.1 [Linux] Clang build issues
4 participants