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

Added static casts of size_t container sizes #3

Closed
wants to merge 1 commit into from
Closed

Added static casts of size_t container sizes #3

wants to merge 1 commit into from

Conversation

w1th0utnam3
Copy link
Contributor

Previously size_t values returned by the internal containers were implicitly converted to the ECS size_type which results in compiler warnings about possible loss of accuracy (if size_type < size_t). If you want to make the user aware of this issue, I would rather add an assert in the create() method or something like this.

Previously `size_t` values returned by the internal containers were implicitely converted to the ECS `size_type` which results in compiler warnings.
@skypjack
Copy link
Owner

At the end of the day, the type of the entities rules on the size and the size type. I added Entity template parameter and forgot to clean up that part. I don't get this PR solves the issue, it's simply a workaround. Let me review it entirely and propose something different.

@w1th0utnam3
Copy link
Contributor Author

I was a little bit confused why size_type is currently fixed to std::uint32_t. And yes, obviously just a workaround - probably not sensible for this general repository in comparison to the toy project I use it for.

@skypjack
Copy link
Owner

I pushed on master a few changes that should clean things up. Can you give me a feedback, please?

@w1th0utnam3
Copy link
Contributor Author

  1. I get the internal compiler error of MSVC again in the base class initialization of ComponentPool (here). I have to experiment to find a workaround.
  2. There is still a potentially lossy conversion here (std::size_t to pos_type)

@skypjack
Copy link
Owner

Oh, I'm sorry, appveyor is having problems for I didn't set master as default branch. I'm going to fix it, not a problem. Give me a couple of minutes.

@w1th0utnam3
Copy link
Contributor Author

w1th0utnam3 commented May 28, 2017

The problem is the usage of the alias template

template<typename Comp>
using Pool = ComponentPool<Entity, Comp>;

in the pack expansion context of the initializer:

explicit ComponentPool(size_type dim = 4098) noexcept
        : Pool<Component>{dim}, Pool<Components>{dim}...

Replacing it with the full class name:

explicit ComponentPool(size_type dim = 4098) noexcept
        : ComponentPool<Entity, Component>{dim}, ComponentPool<Entity, Components>{dim}...

fixes the error.

skypjack added a commit that referenced this pull request May 28, 2017
@skypjack
Copy link
Owner

Push upstream and added -Wconversion to the CMakeLists.txt. From now on, travis will tell us what's wrong in terms of conversions. ;-)

@w1th0utnam3
Copy link
Contributor Author

Cool. Yes, everything seems fine now.

@skypjack
Copy link
Owner

Thank you very much. I'm closing this issue and considering it solved.

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