DM-13750: Move BaseRecord.__str__ implementation to C++. #328
Conversation
src/table/BaseRecord.cc
Outdated
|
|
||
|
|
||
| void BaseRecord::_stream(std::ostream & os) const { | ||
| PrintField functor(this, &os); |
There was a problem hiding this comment.
Personally I would find this much clearer and easier to read if you wrote this as a lambda with a capture on this and os. It would put the function right next to where it was used, and reduce many lines in this file. This seems exactly to be the kind of thing that lambdas were made for. It would also negate the need to get the address of os to pass into the functor, just to keep the size of the functor small, you could simply reference capture os, and have it as a reference all the way down.
Also, if a functor is used, please consider reverse the arguments of BaseRecord and std::ostream to put them in the same order as operator<<. I dont know if it is standard to pass this first, and if so fine, but it seems weirdly asymmetric.
There was a problem hiding this comment.
Good idea!
I was thinking in C++11, which does not permit "universal lambdas" (lambdas with templated function-call operators) instead of C++14, which does.
TallJimbo
force-pushed
the
tickets/DM-13750
branch
2 times, most recently
from
a302b06
to
8803a2d
Compare
Old implementation was very inefficient, even in Python, but we can make it even faster by moving it to C++.
TallJimbo
force-pushed
the
tickets/DM-13750
branch
from
8803a2d
to
c4da012
Compare
Old implementation was very inefficient, even in Python, but we can make it even faster by moving it to C++.