Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-2452

Replace toString() function

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

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

      And after that you can do:

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

      And this has no overhead or any temporary objects created.

        Attachments

          Activity

          Hide
          jbecla 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
          jbecla 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
          jbecla 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
          jbecla 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
          fritzm 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
          fritzm 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
          salnikov 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
          salnikov 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
          jbecla Jacek Becla added a comment -

          I applied all comments, closing.

          Show
          jbecla Jacek Becla added a comment - I applied all comments, closing.

            People

            • Assignee:
              fritzm Fritz Mueller
              Reporter:
              jammes Fabrice Jammes
              Reviewers:
              Andy Salnikov
              Watchers:
              Andy Salnikov, Fabrice Jammes, Fritz Mueller, Jacek Becla
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel