A function for convenient string concatenation with variadic templates

Question

I wrote this function for string concatenation some time ago.

Jo() stands for "Join". The name is shortened on purpose, because this function is used very often.

It offers a stream-like functionality: Jo("i = ", i) = cout << "i = " << i
and returns a const char * to internal buffer.

const char * is used instead of std::string because I interact with C apis a lot and writing .c_str() each time is tedious.

I know that you can achieve same result by converting arguments to std::string one by one and concatenating them instead of using this function, but it would require a lot more code.

It's intended to be used in a single thread only, thus there are no mutextes or something similar.

Here is the code:

#include <cstdio>
#include <sstream>
#include <initializer_list>


namespace Internal
{
    // In real code those are declared `extern` and initialization is moved from .h into .cpp
    std::stringstream ss;
    const std::stringstream::fmtflags stdfmt = ss.flags();
}

// Jo() resets stringstream flags before performing concatenation.
// Jo_() does not.

template <typename ...P> const char *Jo_(P &&... p)
{
    // Multiple buffers to allow use of multiple Jo()'s in a single expression.
    static constexpr int ret_buffers_c = 32;
    static std::string ret_buffers[ret_buffers_c];
    static int ret_pos = 0;

    Internal::ss.clear(); // Clear the error flags.
    Internal::ss.str(""); // Clear stringstream contents.

    // Push all objects to stringstream.
    std::initializer_list<int>{(Internal::ss << p, 0)...};

    // Save to buffer and return a pointer.
    ret_buffers[ret_pos] = Internal::ss.str();
    const char *ret = ret_buffers[ret_pos].c_str();
    ret_pos = (ret_pos + 1) % ret_buffers_c;
    return ret;
}

template <typename ...P> const char *Jo(P &&... p)
{
    Internal::ss.flags(Internal::stdfmt);
    return Jo_((P &&) p...);
}


// Example usage:
int main()
{
    for (int i = 1; i <= 10; i++)
        std::puts(Jo(i, " * ", i+1, " = ", i*i+i));
    return 0;
}

Is there anything I can improve about those functions?

I'm especially concerned about this line: std::initializer_list<int>{(Internal::ss << p, 0)...};
It looks a bit dirty, but I don't know any better alternative.

Answer 1

I don't like the idea in any way, but it is my personal opinion. The approach may bring up problems such as occasional buffer override if your buffer happens to be smaller than needed.

If you really don't want to write c_str() all the time you can wrap std::stringstream with some kind of proxy, that would have operator const char*() as a member function. The only problem is to get all the copies of member operator << of the stringstream. I solved it using a template.

#include <sstream>
#include <string>

class sstream_proxy
{
    std::stringstream stream;
    std::string last;
public:
    template <typename T>
    sstream_proxy& operator<<(const T& value)
    {
        stream << value;
        return *this;
    }

    operator const char*()
    {
        last = stream.str();
        stream.clear();
        stream.str("");
        return last.c_str();
    }
};

The buffer will have the same content unless you call converting operator again. If you want to have multiple buffers, you can have multiple instances of sstream_proxy. The code is not bulletproof, but much easier to read and understand.

The code will be much more natural on its part.

Answer 2

Jo() stands for "Join". The name is shortened on purpose, because this function is used very often.

While it saved you 2 keystrokes when it comes to typing, people who come along to read the code have to search through documentation and source code to figure out what the function does. Just remember,

“Any fool can write code that a computer can understand. Good programmers write code that humans can understand.” -- Martin Fowler, "Refactoring: Improving the Design of Existing Code"

---

Your join function should focus on stringifying arguments together. Move the circular buffer management into its own class.

---

I'm especially concerned about this line:

std::initializer_list<int>{(Internal::ss << p, 0)...};

You should be getting warnings about unused results. If someone you were working with was evil enough to overload the comma operator, that could have a side-effect on the expansion. You can guard against that by casting the result of your each expanded expression to void. Casting to void has the effect of ignoring the result and ensuring the built-in comma operator is used.

(void)std::initializer_list<int>{((void)Internal::ss << p, 0)...};

An alternative is to use a lambda.

(void) std::initializer_list<int>{
    [&](const auto& arg){stream << arg; return 0;}(args)...
};

---

You're pack expansion could be abstracted into a general purpose function that applies a parameter pack to a function.

template <typename UnaryOp, typename... Args>
void for_each_arg(UnaryOp op, Args&&... args) {
  // GCC upto 7.0 doesn't like inlining of uninitialized referenced lambda's
  // in parameter packs.
  auto func = [&](const auto& arg) {
    op(arg);
    return 0;
  };
  (void)std::initializer_list<int>{func(args)...};
}

template <typename... Args>
std::string join(std::ostringstream& reusable_buffer, Args&&... args) {
  for_each_arg([&](const auto& arg) { reusable_buffer << arg; },
               std::forward<Args>(args)...);
  return reusable_buffer.str();
}

template <typename... Args>
std::string join(Args&&... args) {
  std::ostringstream buffer;
  return join(buffer, std::forward<Args>(args)...);
}

I'm pretty sure there are better approaches that use std::tuple, std::integer_sequence, and std::invoke/std::apply.