Tim Armstrong has posted comments on this change. Change subject: Reduce dependencies on inline header functions ......................................................................
Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/2485/4/be/src/exprs/decimal-operators.cc File be/src/exprs/decimal-operators.cc: Line 206: T DecimalOperators::RoundDelta(const DecimalValue<T>& v, int src_scale, > Should we inline this? It looks long but maybe some of the op-specific code Yep, good idea. I am working on another patch that inlines more constants into the decimal code - the scale and precision constants weren't actually replaced in the generated code, I'll do this in that patch. All of the callers are in the module so moving it to the .cc file shouldn't prevent inlining. (not sure if that was the concern) http://gerrit.cloudera.org:8080/#/c/2485/4/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: Line 163: static DateTimeFormatContext* CreateFormatContext(); > Did you mean to include these methods? I don't see their implementations. Deleted. They were leftover from an idea I tried that didn't work :) http://gerrit.cloudera.org:8080/#/c/2485/4/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: Line 16: #include <cstddef> > Out of curiosity, what is this for? At one point NULL wasn't defined in any include from this file but with some of the other changes it's being pulled in again, so I'll remove it. -- To view, visit http://gerrit.cloudera.org:8080/2485 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7a2f388cd14a4427c43af2724340a2ffe8fae3d Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
