# Replace toString() function

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
None
• Story Points:
3
• Sprint:
DB_W16_01
• Team:
Data Access and Database

## Description

See Andy Salnikov comment:

Fabrice, anything is possible in C++, if you can define toString() for vectors it should also be possible to define some other construct to format vector into a stream

My objection to toString() is based on couple of of observations:

most of the time in our code converting complex objects to string is done to push them to streams or to logging system (logging is also usually based of streams)
methods like toString() are usually implemented using temporary streams.

So if you write code like cout << toString(vector) or LOGF_DEBUG("vector: %1%" % toString(vector)) it is very inefficient because it creates temporary stream and temporary string(s).

To make it more efficient you have to define operator<<() which is implemented without using toString(). Then you could implement toString() based on operator<<() but I'd argue that you should avoid it. In case you really need to convert to string there semi-standard tools which already do the same for types that have operator<< defined (like boost::lexical_cast), but again most of the time you only need operator<< as you don't want to mess with strings.

If you want to know how to implement operator<< for vector (or any container) here is the sketch of what I would do (there might be simpler ways):

 namespace detail {  template struct _ContInserterHelper {  const Cont& cont;  };  template std::ostream& operator<<(std::ostream& out, const _ContInserterHelper& cins) {  out << "[";  const Cont& cont = cins.cont; // this is container itself  // print container elements with separators  return out << "]";  } } template detail::_ContInserterHelper ContInserter(const Cont& cont) {  return detail::_ContInserterHelper{cont}; }

And after that you can do:

 std::vector v; std::cout << ContInserter(v);

And this has no overhead or any temporary objects created.

## Activity

Hide
Jacek Becla added a comment -

Andy, can you have a look? I tried to switch away from toString() wherever we are using streams, printing etc. There are still quite a few places where actual std::string is required and I left these as it was. Feel free to suggest deeper cuts.

Show
Jacek Becla added a comment - Andy, can you have a look? I tried to switch away from toString() wherever we are using streams, printing etc. There are still quite a few places where actual std::string is required and I left these as it was. Feel free to suggest deeper cuts.
Hide
Jacek Becla added a comment -

P.S. not sure why jira is not picking up the PR, but it is there! https://github.com/lsst/qserv/pull/183

Show
Jacek Becla added a comment - P.S. not sure why jira is not picking up the PR, but it is there! https://github.com/lsst/qserv/pull/183
Hide
Fritz Mueller added a comment - - edited

Took a look at these changes since I was up and they look good to me. Leaving for AndyS though in case he had something more in-depth in mind?

Show
Fritz Mueller added a comment - - edited Took a look at these changes since I was up and they look good to me. Leaving for AndyS though in case he had something more in-depth in mind?
Hide
Andy Salnikov added a comment -

I have reviewed changes and left one comment in PR. We are moving in the right direction, and I can make few suggestions which are related to this:

• wdb/ChunkResource.cc defines static function std::string toStringLockStatus(LockStatus ls), I think we should replace it with operator<< as well
• drop query::ChunkMapping::_toString() and replace calls to that method with std::to_string()
• what occurs to me is that toString() is rather generic name and in many cases we use this method to do very specific things, e.g. construct proper SQL syntax (without even documenting what that method is expected to return). I'd suggest to rename those uses to something like sqlFragment() to at least convey that method is supposed to be used for that purpose.

I'm marking it as review complete but can re-review if you do more changes.

Show
Andy Salnikov added a comment - I have reviewed changes and left one comment in PR. We are moving in the right direction, and I can make few suggestions which are related to this: wdb/ChunkResource.cc defines static function std::string toStringLockStatus(LockStatus ls) , I think we should replace it with operator<< as well drop query::ChunkMapping::_toString() and replace calls to that method with std::to_string() what occurs to me is that toString() is rather generic name and in many cases we use this method to do very specific things, e.g. construct proper SQL syntax (without even documenting what that method is expected to return). I'd suggest to rename those uses to something like sqlFragment() to at least convey that method is supposed to be used for that purpose. I'm marking it as review complete but can re-review if you do more changes.
Hide
Jacek Becla added a comment -

Show

## People

• Assignee:
Fritz Mueller
Reporter:
Fabrice Jammes
Reviewers:
Andy Salnikov
Watchers:
Andy Salnikov, Fabrice Jammes, Fritz Mueller, Jacek Becla