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

Explicitly instantiate conditional_t #38361

Merged
merged 5 commits into from
Feb 26, 2020

Conversation

kevingranade
Copy link
Member

Summary

SUMMARY: None

Purpose of change

Build time profiling (see https://gist.github.com/ralreegorganon/b8211f6a286b880108ddad810aba76e3#file-gistfile1-txt-L159) points to conditional.cpp compile times being extremely high. It looks like the templating of conditional_t is the culprit.

Describe the solution

Switching conditional_t to explicit instantiation to reduce compile times.

Testing

If it builds it should work.

Additional context

See https://en.cppreference.com/w/cpp/language/class_template for explanation of explicit instantiation.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments labels Feb 26, 2020
@ZhilkinSerg ZhilkinSerg merged commit 4d7b521 into master Feb 26, 2020
@ZhilkinSerg
Copy link
Contributor

Breaks cross-compile to OSX on Jenkins, but don't affect native OSX builds as Travis CI passed.

https://ci.narc.ro/job/Cataclysm-Matrix/Graphics=Tiles,Platform=OSX/10370/consoleFull

13:36:14 Undefined symbols for architecture x86_64:
13:36:14   "conditional_t<dialogue>::conditional_t()", referenced from:
13:36:14       dynamic_line_t::dynamic_line_t(JsonObject const&) in npctalk.o
13:36:14 ld: symbol(s) not found for architecture x86_64
13:36:14 clang: error: linker command failed with exit code 1 (use -v to see invocation)
13:36:14 Makefile:828: recipe for target 'cataclysm-tiles' failed
13:36:14 make: *** [cataclysm-tiles] Error 1
13:36:14 make: *** Waiting for unfinished jobs....

@kevingranade
Copy link
Member Author

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/compiling-error/22951/3

@kevingranade
Copy link
Member Author

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/compiling-error/22951/4

@kevingranade kevingranade deleted the kevingranade-explicitly-instantiate-conditional_t branch March 30, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants