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

strip_credit_from_gid has misleading semantics #717

Closed
brycelelbach opened this issue Feb 15, 2013 · 1 comment
Closed

strip_credit_from_gid has misleading semantics #717

brycelelbach opened this issue Feb 15, 2013 · 1 comment

Comments

@brycelelbach
Copy link
Member

I'm not sure when/why strip_credit_from_cgid was removed, but I'd like to discuss
bringing it back.

The problem with strip_credit_from_gid is that the version which takes gid_type by
reference mutates, and the version which takes it by const ref doesn't. I believe
this violates the principal of least surprise.

For example, I had a class where I defined an equality operator by comparing gids.
I wanted the gids to compare equal even if they had different credit counts, so
I used strip_credit_from_gid. Unfortunately for me, while the equality operator was
a const member function, the gid was a mutable data member (actually, the gid was
a mutable data member of another class, and that class was a data member of my class).
This led all of my client classes to have their credit counts stripped, leading to assertions
during serialization.

Yes, admittedly you get what you ask for when you use mutable data members, but I
could have just as easily shot myself in the foot. I could have easily had a component
action that was non-const but intended and expected a gid that it passed to strip_credit_from_gid
to not be mutated. I think the behavior of an overloaded function like strip_credit_from_gid
shouldn't depend on the constness of the argument you pass to it.

@hkaiser
Copy link
Member

hkaiser commented Feb 15, 2013

What about using the existing bool gid_type::operator==(gid_type const&); in the first place? I'd rather move the function strip_credit_from_gid() into namespace detail as no user is ever supposed to call it.

But I see your point. What about if we rename the const version completely: get_stripped_gid(). This makes it really explicit.

@ghost ghost assigned hkaiser Feb 15, 2013
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

2 participants