[FIX] Incorrect copy/move of fm_index #1144
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1144 +/- ##
==========================================
+ Coverage 96.42% 96.42% +<.01%
==========================================
Files 196 196
Lines 7740 7756 +16
==========================================
+ Hits 7463 7479 +16
Misses 277 277
Continue to review full report at Codecov.
|
| rhs.text_begin_ss.set_vector(&rhs.text_begin); | ||
| rhs.text_begin_rs.set_vector(&rhs.text_begin); | ||
| } | ||
|
|
There was a problem hiding this comment.
Since C++11 Copy'n'swap is typically implemented like this:
- copy constructor with copy implementation
- move constructor with move implementation
- assignment operator that takes by value and std::swaps
- You very rarely need to define a
.swap()member since C++11. Always use std::swap which uses the move constructor automatically.
There was a problem hiding this comment.
I would write it like this:
fm_index(fm_index const & rhs) :
index{rhs.index}, sigma{rhs.sigma}, text_begin{rhs.text_begin}, text_begin_ss{rhs.text_begin_ss}
{
text_begin_ss.set_vector(&text_begin);
text_begin_rs.set_vector(&text_begin);
}
fm_index(fm_index && rhs) :
index{std::move(rhs.index)}, sigma{std::move(rhs.sigma)}, text_begin{std::move(rhs.text_begin)}, text_begin_ss{std::move(rhs.text_begin_ss)}
{
text_begin_ss.set_vector(&text_begin);
text_begin_rs.set_vector(&text_begin);
}
fm_index & operator=(fm_index rhs) //!< Delegate to swap().
{
std::swap(*this, rhs);
return *this;
}This clearly highlights that only thing non-standard that happens is calling set_vector for two members.
There was a problem hiding this comment.
Mhm, using std::swap segfaults. It doesn't seem to set the pointers correctly.
There was a problem hiding this comment.
Hm, that sounds weird. Then again, maybe std::swap uses assignment which uses std::swap ... 🤔
In any case, if it works now and std::swap works now as well, we could also just put move assignment of the members (no swap required) into the assignment operator and get rid of the swap member, or not? I really don't like having a swap member, it looks weird 😇
There was a problem hiding this comment.
Hm, that sounds weird. Then again, maybe std::swap uses assignment which uses std::swap ... 🤔
It kinda looked like it in the debugger.
In any case, if it works now and
std::swapworks now as well, we could also just put move assignment of the members (no swap required) into the assignment operator and get rid of the swap member, or not? I really don't like having a swap member, it looks weird 😇
Ok, done!
| fm_index & operator=(fm_index &&) = default; //!< Move assignment. | ||
| ~fm_index() = default; //!< Destructor. | ||
| fm_index() = default; //!< Defaulted. | ||
| fm_index(fm_index const & rhs) //!< When copy constructing, also set the pointers of rank and select support. |
There was a problem hiding this comment.
Do users of the fm_index (need to) know what rank and select support are?
There was a problem hiding this comment.
When copy constructing, also update internal data structures. ?
eseiler
force-pushed
the
fix/fm_index_cm
branch
2 times, most recently
from
66c1f38
to
7aa94b0
Compare
Resolves #1121