[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Undefined behaviour in range-assign #438

Closed
indianakernick opened this issue Mar 5, 2020 · 5 comments
Closed

Undefined behaviour in range-assign #438

indianakernick opened this issue Mar 5, 2020 · 5 comments
Assignees
@skypjack
Labels
enhancement accepted requests, sooner or later I'll do it

Comments

@indianakernick
Copy link
Contributor

I was reading through the code stumbled upon range-assign at 642 and 661.
I was initially confused by the difference between these functions until I found the relevant unit test. It seems easy to mess these functions up. For example:

#include <iostream>
#include <entt/entity/registry.hpp>

int main() {
  entt::registry reg;
  std::vector<entt::entity> vec {reg.create(), reg.create()};
  reg.assign<float>(vec.begin(), vec.end(), 3);
  std::cout << reg.get<float>(vec[0]) << '\n';
  std::cout << reg.get<float>(vec[1]) << '\n';
}

Can you guess what the output of this program is?

5
5

I don't really know how 3 is being turned into 5.0f. If you try some different numbers you get some interesting output. Assigning 1 results in 3 0.

Maybe that example is a bit contrived (I'm not really using the function as intended and primitive types are rarely used as components) but the fact that it's even possible to do this without any assertions or compiler errors is a hole in the API.
@indianakernick indianakernick changed the title Difficult to detect undefined behaviour in range-assign Difficult-to-detect undefined behaviour in range-assign Mar 5, 2020
@indianakernick indianakernick changed the title Difficult-to-detect undefined behaviour in range-assign Undefined behaviour in range-assign Mar 5, 2020
@skypjack skypjack self-assigned this Mar 5, 2020
@skypjack skypjack added the triage pending issue, PR or whatever label Mar 5, 2020
@skypjack
Copy link
Owner
skypjack commented Mar 8, 2020

I think I'll rename the basic assign to assign_from_args and reintroduce a replace_from_args to address #437. This should help to disambiguate the two cases where the template parameter pack makes things odd.
what do you think about?

@skypjack skypjack added enhancement accepted requests, sooner or later I'll do it and removed triage pending issue, PR or whatever labels Mar 11, 2020
skypjack added a commit that referenced this issue Mar 11, 2020
@skypjack
storage: disambiguate calls to ::construct (see #438)
d8f2891
skypjack added a commit that referenced this issue Mar 11, 2020
@skypjack
sparse_set: disambiguate calls to ::construct (see #438)
09ee468
skypjack added a commit that referenced this issue Mar 11, 2020
@skypjack
registry: disambiguate calls to pool_handler<T>::construct (see #438)
9a3b585
@skypjack
Copy link
Owner

I introduce new functions to disambiguate the calls you mentioned. In particular: patch, emplace, insert and emplace_or_replace.
Old functions are still there but have been deprecated (as in [[deprecate("...")]]). They will be removed in future.

Let me know if this is the expected result. Ambiguity on assign has been drastically reduced imho. Moreover, the migration is smooth this way, since assign has been most likely largely used so far by all users of EnTT. 👍

@indianakernick
Copy link
Contributor Author
indianakernick commented Mar 12, 2020 •
edited
Loading

I updated my example (assign -> insert) and I still get the same result (3 becomes 5.0f). There is still ambiguity between the two overloads of insert. My example is probably too contrived to be worth addressing but I do like the new names.

It would probably be enough to ensure that CIt is actually an iterator because at the moment, it is unconstrained.

skypjack added a commit that referenced this issue Mar 12, 2020
@skypjack
registry: strict check on ::insert (see #438)
5e04f0a
@indianakernick
Copy link
Contributor Author

After seeing the latest change, I'm convinced that this issue has been thoroughly resolved. The API is much clearer and easier to understand. Simply renaming some functions has had a huge impact. This is really great! You can close this when the changes are merged.

@skypjack
Copy link
Owner

Yup. I agree. Good suggestions are good suggestions, no doubts about it. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@skypjack skypjack

Labels
enhancement accepted requests, sooner or later I'll do it
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@skypjack @indianakernick