oddly named layer::swap #1652

Closed
joto opened this Issue Dec 14, 2012 · 6 comments

Projects

None yet

3 participants

@joto
Contributor
joto commented Dec 14, 2012

In src/layer.cpp the function layer::swap does't do a swap! It just does a copy.

@springmeyer
Member

it appears to have long been used in the copy ctor - feel free to provide a pull with it renamed to something more reasonable. Thanks!

@artemp
Member
artemp commented Dec 14, 2012

Name is good, don't rename it, please :)

@joto - it's pretty much standard name when implementing exception safe assignment operator, see "C++ Coding Standards: 101 Rules, Guidelines, And Best Practices"

@springmeyer - assignment operator not copy constructor

@joto
Contributor
joto commented Dec 14, 2012

@artemp - Yes, I am familiar with that idom. But that doesn't change the problem. The swap function doesn't actually implement what it says it does. It doesn't do a swap. This might not matter in this case, because it might be not ever be used in a different context than the assignment ctor, but then again, it might.

@artemp
Member
artemp commented Dec 18, 2012

@joto - I can see where are you coming from:) I can't think of a better name, "swap" is used in a few text books, though.
Also, it's a private member function so shouldn't be abused anyway.

@joto
Contributor
joto commented Dec 20, 2012

I wrote a patch that should fix this problem correctly. It is at http://www.remote.org/jochen/tmp/swap.patch . Sorry, no pull request, but I already have a fork of mapnik and can't fork the repository twice. I am sure there is a way to do this properly with git, but I haven't got the right magic spell.

@artemp
Member
artemp commented Dec 20, 2012

@joto - applied in 6512d28, cheers!

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