Refactor of RpcapdServerInitializer #1410
Conversation
| } | ||
| public: | ||
| RpcapdServerInitializer(bool activateRemoteDevice, const std::string& ip, uint16_t port) | ||
| : m_ProcessHandle(nullptr), m_JobHandle(nullptr) | ||
| { | ||
| if (!activateRemoteDevice) | ||
| return; | ||
|
|
||
| std::string cmd = "rpcapd\\rpcapd.exe"; |
There was a problem hiding this comment.
we can move this line to around L163 (new)
There was a problem hiding this comment.
True, tho IMO I prefer the variables being initialized in the sequence they are used in the CreateProcess call.
|
@seladb I think this is ready for review. |
| RpcapdServerInitializer(RpcapdServerInitializer&& other) noexcept | ||
| : m_ProcessHandle(other.m_ProcessHandle), m_JobHandle(other.m_JobHandle) | ||
| { | ||
| other.m_ProcessHandle = nullptr; | ||
| other.m_JobHandle = nullptr; | ||
| } | ||
| RpcapdServerInitializer& operator=(const RpcapdServerInitializer&) = delete; | ||
| RpcapdServerInitializer& operator=(RpcapdServerInitializer&& other) noexcept | ||
| { | ||
| killProcessAndCloseHandles(); | ||
| m_ProcessHandle = other.m_ProcessHandle; | ||
| m_JobHandle = other.m_JobHandle; | ||
| other.m_ProcessHandle = nullptr; | ||
| other.m_JobHandle = nullptr; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Do we really need all of these methods? We don't the copy c'tor or the assignment operator anywhere
There was a problem hiding this comment.
Well, the class is not safe to copy, but since it only holds POD members, a copy ctor will be implicitly declared by the compiler if not explicitly deleted. As for the move ctor, @tigercosmos wanted them added as deleted too, but since the class is technically movable, I decided to write them out instead of declaring them deleted.
There was a problem hiding this comment.
ok, I have no specific objection. It's a helper class in our test suite so it doesn't have to be perfect 🙂
| } | ||
| public: | ||
| RpcapdServerInitializer(bool activateRemoteDevice, const std::string& ip, uint16_t port) | ||
| : m_ProcessHandle(nullptr), m_JobHandle(nullptr) | ||
| { | ||
| if (!activateRemoteDevice) | ||
| return; | ||
|
|
||
| std::string cmd = "rpcapd\\rpcapd.exe"; |
There was a problem hiding this comment.
nit: I don't remember why I defined this string twice (here and in line 127). Maybe we can use cmd when we write args?
This PR refactors the
RpcapdServerInitializerobject and contains the following changes:getHandlemethod marked asconst.RpcapdServerInitializer.NB: Move constructor can be added at a later date if needed.
ostringstreamin favor of straightstringallocation withreserve+append.