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

Remove get_type_index in favour of more standard which() #73

Open
artemp opened this issue Jan 27, 2016 · 6 comments
Open

Remove get_type_index in favour of more standard which() #73

artemp opened this issue Jan 27, 2016 · 6 comments

Comments

@artemp
Copy link
Contributor

artemp commented Jan 27, 2016

get_type_index returns index in reversed order which is confusing.
which method is complaint with boost::variant.

In order to remove get_type_index we need to address last remaining issue - invalid_type value.

Proposed solution:

invalid_type == sizeof(Types...) + 1
// relevant test that needs upgrading
REQUIRE(variant_type{mapbox::util::no_init()}.get_type_index() == mapbox::util::detail::invalid_value);
@lightmare
Copy link
Contributor

I was thinking about the invalid index value when attempting to use a smaller type for storing the index (#19).

std/tuple/boost/... use indices 0, ..., N-1, and size_t(-1) as invalid index. I think of that as the invalid type preceding valid types in the list (its index is -1, although unsigned).

mapbox::variant internally uses indices N-1, ..., 0, and size_t(-1) as invalid. In the same line of thinking, the invalid type follows valid types. index_type(-1) is awkward in comparisons when index_type is an arbitrary unsigned type, so while implementing that, I modified the internal indexing to N, ..., 1, and 0 as invalid index. It's simple and reliable with whatever type is used for storing the index.

@artemp
Copy link
Contributor Author

artemp commented Jan 28, 2016

@lightmare - thanks. I can see it will work but I'm a bit hesitant to just go and change internal index to
N,...,1 Lets get a consensus on this :) @joto ?

@springmeyer springmeyer added this to the 1.1.0 milestone Jan 28, 2016
@springmeyer
Copy link
Contributor

@artemp how is this related to #67? Can one be closed in favor of the other?

@joto
Copy link
Contributor

joto commented Jan 29, 2016

Currently there is still the public get_type_index() function, so this internal index is visible outside. I suggest we mark this function as deprecated and remove it in 2.0 and then revisit this issue. @artemp?

@artemp
Copy link
Contributor Author

artemp commented Jan 29, 2016

ok with me

/cc @joto @lightmare @springmeyer

@lightmare
Copy link
Contributor

/agree, shouldn't be changed until it's truly internal.

joto added a commit that referenced this issue Jan 29, 2016
@joto joto modified the milestones: 2.0.0, 1.1.0 Jan 29, 2016
springmeyer pushed a commit to mapnik/mapnik that referenced this issue Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants