-
-
Notifications
You must be signed in to change notification settings - Fork 262
Merge putDtype and putType; remove duplicated code #8610
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
Merge putDtype and putType; remove duplicated code #8610
Conversation
asfernandes
left a comment
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.
If you not choose better names for methods you created to not duplicate code, I prefer to have the code duplicated.
I understand nothing what the refactored methods do by their names.
I tried to follow the style of the original method names. |
| } | ||
|
|
||
| void DsqlCompilerScratch::putType(const TypeClause* type, bool useSubType) | ||
| template<bool THasTableName> |
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.
What means THasTableName?
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 indicates the presence of typeOfTable.* fields in the TypeClause& type. That is, we use blr_column_name (i.e., the table name with typeOfTable) or blr_domain_name (with typeOfName).
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.
I'd question the need of the T prefix, as it usually means some type used as the template argument, not the variable. Simple HasTableName or maybe IsTypeOfTable is IMHO better readable.
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.
It would be nice to have some explicit coding style for such cases. The existing one suggests using a single letter, which is not the clearest name possible in this case. Personally, I use the T prefix to make the template argument easier to recognize in the code, especially with if constexpr statements
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.
I'd question the need of the
Tprefix, as it usually means some type used as the template argument, not the variable. SimpleHasTableNameor maybeIsTypeOfTableis IMHO better readable.
Yes.
| DsqlCompilerScratch* makeScratch() | ||
| { | ||
| auto& pool = *getDefaultMemoryPool(); | ||
| return FB_NEW DsqlCompilerScratch(pool, nullptr, nullptr); |
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.
Why create objects in heap and leak them instead of use local objects and let RAII destruct them?
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.
Oops, I forgot to fix my raw temporary implementation
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.
Oh, this is why I am leaking memory:
protected:
// DsqlCompilerScratch should never be destroyed using delete.
// It dies together with it's pool.
~DsqlCompilerScratch()
{
}
I can create a derived class and use it, but I'm not sure if it's safe to delete it, since the DsqlCompilerScratch has a strict prohibition
| } | ||
|
|
||
| void DsqlCompilerScratch::putType(const TypeClause* type, bool useSubType) | ||
| template<bool THasTableName> |
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.
I'd question the need of the
Tprefix, as it usually means some type used as the template argument, not the variable. SimpleHasTableNameor maybeIsTypeOfTableis IMHO better readable.
Yes.
This is also a preparation for the JSON Type implementation