Hot answers tagged

11

I agree with Loki's answer: the difference between calling close explicitly, and letting the destructor call close, is that the destructor will implicitly catch (i.e. conceal) any exception thrown by close. The destructor must do this (not propagate exceptions) because it may be called if/while there is an exception already being thrown; and throwing a 2nd ...


7

I believe you are asking two questions in one. Should you use exceptions or return values? Should you use RAII or not? If exceptions are not permitted in your company, then fstream::exceptions() must be set globally for your project. And you have to interrogate the error flag too. If RAII is not permitted in your company, then do not use C++. If you use ...


7

Let me first assume that your unique_ptr is supposed to be movable. Then, any basic test case whould have unrevealed this: unique_ptr<int> ptr1(new int()); unique_ptr<int> ptr2 = std::move(ptr1); // Fails to compile Recall that = delete-ing special member functions means user-declaring them. And user-declared copy and copy assignment ...


7

These are good questions! 1. You should indeed have the copy constructor & assignment operator deleted. The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-...


6

I've only had a chance to glance at things so far, but this almost jumped out at me: bool operator==(Vector const& rhs) const { return (size() == rhs.size()) ? std::equal(begin(), end(), rhs.begin()) : false; } This seems rather convoluted, at least to me. I'd prefer: bool operator==(Vector const& rhs) ...


6

There's one major problem with the code: Your move assignment is broken. You forgot to close(_s) before overwriting it with other._s. To avoid this kind of problems, I suggest using the copy-and-swap idiom. It makes writing a operator= a no-brainer in most cases: tcp_socket &operator=(tcp_socket other) noexcept // Note the lack of `&&`. { ...


5

We can simplify that a lot. You are not really using RAII on the CriticalSection as you declare it in one place then initialize it in another. But CRITICAL_SECTION is a type as you pass a pointer around. Why not simplify a bit: class GetLock; class CriticalSection { public: CriticalSection() { InitializeCriticalSection(&m_CS); // get a ...


5

Construction In your constructor from iterators: template <class It> Simple_Array(It first, It last) : Simple_Array(std::distance(first, last)) { unchecked_copy(first, last); } You do two things: first you default-construct a n objects, and then you copy-assign them. This is both inefficient and reduces the usability of your class. What if ...


5

Great that you provided a docstring! You made one minor formatting mistake though, there should be a blank line between your brief single line summary and the rest of the docstring. It's recommended in the style guide partially for readability and partially for script parsers. class MongoDB(object): """Provides a RAII wrapper for PyMongo db connections. ...


4

When in Qt, act like Qt. Qt already has an ownership model, and it works by passing the parent object (as a raw pointer) in the constructor (so there's no window of exception-triggered leakage). And Qt has its own weak pointers (including QObjectPointer). I wouldn't try to outsmart Qt on pointer lifetime and ownership - but keep watching development, and ...


4

You have: bool valid() const { return ptr == nullptr; } I think you meant != there. Regardless, you probably don't need this method; and if you must have it, consider spelling it either operator bool() const { return ptr != nullptr; } // or else bool operator==(std::nullptr_t) const { return ptr != nullptr; } Try the following program: int main() { ...


4

There's not a lot of code here to be reviewed, but I'll have a go. A small efficiency gain is possible, by moving the db_ argument in the initializer list, rather than copying it: SqlTransaction(std::shared_ptr<sqlite::database> db_) : db{std::move(db_)} { *db << "begin;"; }; It's not necessary to delete the move constructor, as explicitly ...


4

As other answers have said, use swap to get assignment correct, and don't close already-closed descriptors. Some minor style nitpicks to add: Use initializers in preference to assignment in the constructors. There's no need to name the arguments to the deleted copy methods. Remove the redundant parens from return *this;. Consider a public close() method, ...


3

template< typename T, typename _TMtx, Get off the standard library's lawn! _Ugly names are reserved to the implementation. template<typename> class _TLock, template<typename> class _TMutLock > class lock_wrap_impl { template<typename W> using mimic_unique = typename std::conditional< std::...


3

Almost always auto. Only write down the specific type when you have to, it's shorter, clearer, and less error-prone. You currently handle an empty list as a special case. Don't, it's neither efficient nor elegant. Instead, use a pointer to pointer, and everything is simplified. As an example: void push_back(const Type& data) { auto p = &head; ...


3

I think you have a potential usage issue with wrapping these methods in a class. While it's convenient that the Class_Terminate method will automatically re-enable updating, it makes keeping track of the usage of such an object the responsibility of the caller (or calling module or of the entire project). Where you can easily get into trouble is when you ...


3

I would point out that the naming convention is inconsistent -- we have Activate and a Shutdown --- they don't say the same thing to me. I'd expect corresponding actions to have names like Activate -> Deactivate, Enable -> Disable, Startup -> Shutdown. Given the nature of the class, Enable/Disable pair seems the best suited for what your class is going to do,...


3

Error Handling No error handling in construct. I don't think throwing a exception would be appropriate so it's up to the user to check valid(). No. That is a bad choice. If the constructor completes you should have a valid object. Using a method to check if an object is valid is just as bad as checking an error code. It is prone to not be used. If your ...


3

Your concurrency/integrity of the code, and the way you have shown it used, are fine. The design, on the other hand, is very questionable.... I would recommend that you do not use this pattern at all. If you are going to use it (your comment here says it is used already), then I recommend changing the name. It is not a wrapper, since you can't actually ...


3

You didn't give a terrible amount of code to be reviewed, and I am not really proficient in concurrency and the like but I still got a few points I want to make. But enough of introduction let's jump into the code: public class RWLockWrapper implements AutoCloseable { Wonderful, concise and clear. RWLockWrapper is a beatiful name for that class. You could ...


3

first I suggest you use pointers to keep the mutex reference; this makes it possible to have a non-owning state when the pointer is 0 (the state when default initialized) also you created a copy constructor and a destructor, so you still need a copy assignment to properly follow rule of 3 (or at least disable it) GetLock<TLOCKABLE>& operator=(...


3

After reading through the question and the answers I came to the conclusion that this is were comments can come into play. I had recently read this Q/A Guessing a number, but comments concerning and the accepted answer gave me insight to the situation here. Use comments to explain the why, let the code explain the how. I can use your function similarly for ...


3

You also need to check that the write operations (<<) succeeded. So instead of checking is_open(), just go through the whole series of operations, and check failbit at the end: bool save_data() const { std::ofstream saveFile(m_filename); saveFile << m_member1 << '\n' << m_member2 << '\n'; saveFile....


3

C++11 If your compiler has proper C++11 support or better, you don't need, and shouldn't, use the stuff from std::tr1. shared_ptr is a member of namespace std and lives under the include file <memory>. Global init/shutdown Your approach is valid, it ensures proper termination if an exception is thrown and never caught. Still you have to be careful ...


3

Are you sure also scoping de-initialization of Python to main (by removing static) is not an option? As-is, you cannot use Python in a globals initializer or destructor anyway. Having taken a look at Py_Finalize, I actually would not deinitialize it at all, at least not in a release-build, as none of the reasons that option is provided apply to you: This ...


3

Everything Barry said. In addition there are a couple of types I would add. template <typename Ty> class Simple_Array { public: typedef Ty value_type; typedef Ty& reference; typedef Ty* pointer; typedef Ty const& const_reference; typedef Ty const* ...


3

unique_ptr has capability for custom deleters. Besides that assuming you are doing this to deal with COM objects then you can use ComPtr and its GetAddressOf If you define a destructor you should also define (or disallow in this case) the copy construct and copy assign. Failure to do so will result in dangling pointers and double frees: Releaser<...


3

Access I don't like the fact that Access gives you a reference to the object directly. This leads to issues with somebody accidentally keeping a reference to the object after the Accesses object is destroyed or accidentally making a copy of the object. T& myRef = wrapped.access().obj_; // Now I have a reference to the object ...


3

Ensuring you have distinct types You said this in the comments: I thought that use of templates instead of a common base class would better emphasize that these objects shouldn't be mixed together (now they are completely distinct types). Two classes that derive from the same base class are also two distinct types. Furthermore, you can make the constructor ...


3

One tiny improvement I'd suggest is that the stored state can all be declared const: const std::ios_base::fmtflags flags; const std::streamsize width; const std::streamsize precision; const charT fill; There are no implications for assignability of fmtstash objects, as we already had a reference member.


Only top voted, non community-wiki answers of a minimum length are eligible