-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tickets/dm 45548 #884
base: tickets/DM-43715
Are you sure you want to change the base?
Tickets/dm 45548 #884
Conversation
@@ -233,6 +233,11 @@ std::string UserQuerySelect::getResultQuery() const { | |||
|
|||
/// Begin running on all chunks added so far. | |||
void UserQuerySelect::submit() { | |||
auto exec = _executive; | |||
if (exec == nullptr) { | |||
LOGS(_log, LOG_LVL_ERROR, "UserQuerySelect::submit() executive is null at start"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing. Shouldn't the user query object be constructed with a valid pointer to the "executive"? And if so then why is this test? What makes the pointer to suddenly disappear?
I'm also seeing the very same pattern of checking for the validity of a pointer all around the code of the PR. Perhaps there is a safer way of constructing objects to avoid polutting the code with these tests?
My other worry is that if there is such an uncertainty (with the elusive pointer) then such code must be really hard to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_executive being null at this point should really never happen, so the check can be removed, but the check is harmless and consistent with other UserQuerySelect methods. UserQuerySelect::discard() sets _executive to null (this has been around for a very long time) so the _executive pointer isn't safe to use directly and must be copied. Changing discard() to not reset _executive had side effects, so I'd rather not mess with it at this time.
I have one general comment on redefining the types of the Standard Library. I think it is not a very good idea to introduce an extra layer of indirection for the pure (where no custom template parameters are involved) Consider redirections for
One can quickly see the first problem here in a way Now, let's look at where some of these redefined types are used in
This code is totally confusing. It causes a reviewer to look into the header where both types are defined. Compare this with the clean code w/o type redefinition:
That (second version of the) code is very easy to read (to someone who knows what the Moreover, your next step in streamlining and improving the code would be to put those 2 lines into a function:
And since the function returns the final type then the recipient code would be reduced to use
That code is very easy to read and understand. The bottom line is that when you redefine Standard types then you in essence introduce your own (non-standard) vocabulary. This adds extra complexity to the code on top of your application-specific complexity and vocabulary. It's better not to do this if possible. |
No description provided.