-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
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.
Looks good, some typos
|
||
BuilderExpr bwOr(const BuilderExpr& rhs) const; | ||
BuilderExpr bwOr(int val) const; | ||
BuilderExpr bwOr(int64_t val) const; |
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.
Should we use explicit bit width everywhere? I'd expect either int32_t
with int64_t
or int
with long long
; the latter should be incorrect in our case.
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.
int
and int64_t
overloads match all other similar functions. We can use int32_t
and int64_t
instead, but it should be done globally then, not only for bitwise operations.
Signed-off-by: ienkovich <[email protected]>
51c9b4a
to
ac8cc25
Compare
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.
Looks good. Should we include any arguments restrictions/assumptions in the inline doc?
Covers C++ and PyHDK API but not Calcite.
Related to #542.