current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have a C++03 library with lots of classes. I'm interested in trying to take advantage of Rvalues and move semantics under C++11 for two or three critical classes. Once of the critical classes is a SecBlock, which is effectively a secure array class. It has a secure allocator and deallocator, and its used nearly everywhere.

SecBlock looks like so, where T might be a byte or an int:

template <class T, class A = AllocatorWithCleanup<T> >
class SecBlock
{
   ...
private:
    A m_alloc;
    size_type m_size;
    T *m_ptr;
}

If I am reading Thomas Becker's "C++ Rvalue References" treatise correctly, I can use a simple swap because SecBlock does not have a base class (no need for Base(std::move(rhs)):

template <class T, class A = AllocatorWithCleanup<T> >
class SecBlock
{
   ...

#if CXX11_AVAILABLE
    SecBlock(SecBlock&& t)
    {
        std::swap(*this, t);
    }
    SecBlock& operator=(SecBlock&& t)
    {
        if(this != &t)
            std::swap(*this, t);
        return *this;
    }
#endif

private:
    A m_alloc;
    size_type m_size;
    T *m_ptr;
}

We guard on CXX11_AVAILABLE because the library is a C++03 library at heart.

It's probably obvious I have some open questions. For instance, do I have to guard for self assignment in operator=? (I suspect not, but I think its best to show the code and take the comments).

I'm also interested in knowing if a derived class must implement the semantics also, even if they add no members.

Is the code above correct, or are there gaps that need to be addressed? If there are gaps, what are they?

share|improve this question

put on hold as off-topic by Loki Astari, Hosch250, Barry, Quill, jacwah 17 hours ago

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Loki Astari, Barry, jacwah
  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example. Questions seeking an explanation of someone else's code are also off-topic." – Hosch250, Quill
If this question can be reworded to fit the rules in the help center, please edit the question.
    
Note crypto++ is synonymized with cryptography. –  Mast yesterday
    
@Mast: they are two different things. One is a field of study, the other is a software library. Intuitively, they cannot be used interchangeably in a sentence, so they are clearly not synonyms. –  jww yesterday
3  
This discussion has been held before: Current synonymization of crypto++. We will not have this discussion in comments. –  Mast yesterday
    
@Mast - I'm not interested in a discussion about it. In the previous question, I simply asked what I should do because I was receiving inappropriate/unwanted emails due to the intersection with the notification system. The discussion you folks had about it was irrelevant to me because you discussed something I did not ask. In this question I simply pointed out the errant behavior to let folks know it was not me mis-classifying the question. In both cases, I'm not interested in a discussion. –  jww yesterday
    
Somewhat related, definitely worth the read, nevertheless: stackoverflow.com/a/3279550/1198654 –  glampert yesterday

1 Answer 1

up vote 5 down vote

This code does not function.

Let's take a look at your move constructor: 

SecBlock(SecBlock&& t)
{
    std::swap(*this, t);
}

This caused a stack overflow on my system. Why? Imagine if the swap function was implemented like this:

template <typename T>
void swap (T &t1, T &t2)
{
    T temp = std::move (t1) ;
    t1 = std::move (t2) ;
    t2 = std::move (temp) ;
}

In fact, this is how it is implemented by Visual Studio 2012. The first line T temp = std::move (t1) ;, calls your move constructor. This causes you to recursively call your move constructor until your program inevitably crashes.

Another problem is that before the swap, m_size and m_ptr for *this will be uninitialized. So even if you fixed your move constructor, you will be transferring uninitialized data to t. Assuming you're destructor does some clean-up, this could lead to undefined behavior.

All in all, both member-functions are a mess. Check out the rule of 5 for more help.

share|improve this answer
    
Now I'm in a bad spot. The question may violate the site's policies on working, bug free code. What should I do with it? Delete it, or leave it and accept your answer? (Sorry to ask. I don't make it to these parts often). –  jww yesterday
    
@jww Give it time I guess. If it violates this site's rules, then I imagine the question will eventually be closed. If it doesn't violate the rules, then wait for more answers and select the best one. –  ncalmbeblpaicr0011 yesterday
    
"This code does not function...." - oh, my. It tested OK on OS X. I did not start a testing cycle on Lnux, BSD or Windows. I was waiting for feedback before the testing cycle. Thank you very much. I just learned a tremendous amount about these things... –  jww yesterday
    
I see what happened here.... Apple/Clang claimed 201103L, but failed a feature test. When it failed a feature test, the code path was not activated. When I cut-int std::move without the guards to test your findings, it could not even compile. OS X is the pits. I'm jumping over to Fedora 22 and the GCC 5.1 compiler. –  jww yesterday
    
@jww, if an answer finds a bug in your code, it doesn't automatically makes the question of-topic (in fact it happens quite often that bugs are found by reviews!) If to the best of the asker's knowledge the code is working, then it is on topic. This is the case with your question. You thought it was correct, but a review found a bug. Great thing you asked for code review! –  glampert yesterday

Not the answer you're looking for? Browse other questions tagged or ask your own question.