3
\$\begingroup\$

This is following up on this question

In this part, I wanted to be able to choose arguments by index and have a placeholder for the format of the argument. As usually happens, my initial implementation was pretty much straightforward. Unfortunately, things get a bit ugly from here…

#include <charconv>
#include <iostream>
#include <limits>
#include <sstream>
#include <string>
#include <string_view>
#include <vector>

namespace jdt
{
    struct Segment
    {
        Segment(std::size_t offset, std::size_t count, std::size_t argument = std::numeric_limits<std::size_t>::max(), bool braceError = false)
            : offset{ offset },
            count{ count },
            argument{ argument },
            braceError{ braceError }
        {
        }

        std::size_t offset;
        std::size_t count;
        std::size_t argument;
        bool braceError;
    };

    struct Argument
    {
        Argument(const std::string& format)
            : format{ format },
            handled{ false }
        {
        }

        std::string format;
        std::string text;
        bool handled;
    };

    template<typename T>
    void format_helper(std::vector<Argument>& arguments, std::size_t& index, const T& value)
    {
        if (index < arguments.size())
        {
            Argument& arg = arguments[index];
            std::ostringstream oss;
            oss << value;
            arg.text = oss.str();
            arg.handled = true;
            index++;
        }
    }

    template<typename... Targs>
    std::string format(std::string_view str, Targs...args)
    {
        // PHASE 1 - parse format string
        std::vector<Segment> segments;
        std::vector<Argument> arguments;
        std::size_t offset = 0;
        std::size_t index = str.find('{');
        while (index != std::string::npos)
        {
            // check for literal open-braces character
            if (str[index + 1] == '{')
            {
                segments.emplace_back(offset, index - offset + 1);
                offset = index + 2;
            }
            else
            {
                // check if the next part is an argument index
                std::size_t argindex = arguments.size();
                std::size_t startOfArg = index;
                auto ret = std::from_chars(str.data() + index + 1, str.data() + str.size(), argindex);
                index = ret.ptr - str.data();
                // find closing brace
                bool braceError = false;
                std::size_t closeBrace = str.find('}', index);
                if (closeBrace == std::string::npos)
                {
                    braceError = true;
                    closeBrace = str.size();
                }
                // check for format specifier
                if (str[index] == ':')
                    index++;
                // add to vectors
                arguments.emplace_back(std::string(str.substr(index, closeBrace - index)));
                segments.emplace_back(offset, startOfArg - offset, argindex, braceError);
                offset = closeBrace + 1;
            }
            index = str.find('{', offset);
        }

        // PHASE 2 - Recursive variadic template
        index = 0;
        (format_helper(arguments, index, args), ...);

        // PHASE 3 - Stich the string back together
        std::ostringstream oss;
        for (auto& segment : segments)
        {
            oss << str.substr(segment.offset, segment.count);
            if (segment.braceError == true)
            {
                oss << "{ error: no matching '}' found";
            }
            else
            {
                if (segment.argument != std::numeric_limits<std::size_t>::max())
                {
                    if (segment.argument < arguments.size() && arguments[segment.argument].handled)
                        oss << arguments[segment.argument].text;
                    else
                        oss << format("{{ error: invalid arg index: {} }", segment.argument);
                }
            }
        }
        if (offset < str.size())
            oss << str.substr(offset);
        return oss.str();
    }
}


int main()
{
    int a = 5;
    double b = 3.14;
    std::string c("hello");

    // simple arguments
    std::cout << jdt::format("a = {}, b = {}, c = {}\n", a, b, c);

    // arguments with indexes
    std::cout << jdt::format("a = {2}, b = {1}, c = {0}\n", a, b, c);

    // literal open-braces character
    std::cout << jdt::format("let object = {{\"a\": {0}, \"b\": {1}, \"c\": \"{2}\"};\n", a, b, c);

    // too few arguments
    std::cout << jdt::format("a = {0}, b = {1}, c = {2}\n", a, b);

    // invalid argument indexes
    std::cout << jdt::format("a = {0}, b = {10}, c = {2}\n", a, b, c);

    // too many arguments
    std::cout << jdt::format("a = {2}, b = {1}, c = {0}\n", a, b, c, 3, 4, 5);

    // missing closing brace }
    std::cout << jdt::format("a = {2}, b = {1}, c = {0\n", a, b, c);
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Throw exceptions on format errors

In your code, an error in the format string or the number of arguments will cause jdt::format() to return a valid string anyway, just with the error message inside the returned string. This is very bad, as it will just let the program continue with incorrect output without the program being able to notice or handle the error in any way.

Since the intended usage is to be able to write code like std::cout << jdt::format(...), the best way to report an error is to throw an exception, just like std::format() does.

Fail fast

While scanning the format string for opening and closing braces, the moment you know you are missing a closing brace you can throw an exception. Furthermore, before calling format_helper(), you can check that sizeof...(Targs) >= arguments.size(), and if that is false, again throw an exception.

Pass format arguments by const reference

To avoid making copies of the arguments, make sure they are passed by const reference, like so:

template<typename... Targs>
std::string format(std::string_view str, const Targs&... args) {
    ...
}

Avoid the stiching phase

In your code, in phase 2 you are converting all the arguments to std::strings, and then in phase 3 you are building the final output. This might save some effort in the rare case that you are referencing the same argument multiple times in the format string, but now you are using more memory since you have to first store the literal parts of the format string and the converted arguments in vectors. I would rewrite the code so that you build the final result in one go, by alternatively adding a literal section of the format string, then format an argument, then the next literal section, and so on.

Also note that your approach of first building the vector arguments will no longer work as soon as you start implementing format specifiers, as then you can have the same argument formatted twice but with different format specifiers.

The problem, as you've already noticed of course, is how to be able to do random access to the parameter pack args. This can be solved in a different way:

Run-time indexing of arguments

You might have a look at how std::format handles this problem. In particular, it introduces the class std::format_args to store the arguments before they are converted to strings, and provides a get() member function to access them by index. There is also a helper function std::make_format_args() that you can pass args... and which will return a std::format_args object. But if you actually look at the implementation, you'll see that this is a rabbit hole that goes very deep.

A simpler solution that will do for now is to use fold expressions again, but now use it to build the equivalent of a for-loop to visit each argument until we reach the desired one, and then write it to the output. For example:

template<typename... Targs>
static bool format_one(std::ostream &out, size_t i, const Targs&... args) {
    auto visitor = [&](const auto& arg) {
        if (!i--) {
            out << arg;
            return true;
        } else {
            return false;
        }
    };

    return (visitor(args) || ...);
}

Then you could call it like so:

template<typename... Targs>
std::string format(std::string_view str, const Targs&... args) {
    std::ostringstream oss;
    ...
    for (/* each format specifier in str */) {
        std::size_t argindex = /* get the argument's index */;
        ...
        if (!format_one(oss, argindex, args...)) {
            /* handle the argument not being found */
        }
    }
    ...
}
\$\endgroup\$
6
  • 2
    \$\begingroup\$ In the first iteration, I advised against erroring with incorrect format, because the format string may well come from a translation database, rather than from a programmer. If we throw an exception, its content needs to be specific enough to identify the faulty string and the actual problem detected. \$\endgroup\$ Oct 30, 2021 at 12:04
  • 1
    \$\begingroup\$ My initial thought was also to simply throw an exception and be done with it but @TobySpeight had a valid point. I’m not sure which way will be best. Perhaps having two different functions, one throwing and one not? Re. Run-time indexing of arguments, in hindsight that makes much more sense than my halve baked stitching scheme =) \$\endgroup\$
    – jdt
    Oct 30, 2021 at 14:06
  • \$\begingroup\$ I think that two different functions would be the worst of all worlds! One thing you might do is have one function whose behaviour can be changed at run-time (e.g. via environment variables) for translators' convenience. But exception might be a good way to go, if there's sufficient information conveyed. \$\endgroup\$ Oct 30, 2021 at 17:07
  • \$\begingroup\$ I recommend failing hard if there is a format string error. This is both the right thing for translators and for end-users. Consider that someone who doesn't speak English and depending on the translation might not understand an English error message being inserted into the output. They are then left with a partial message in their own language, which might be confusing. Consider seeing "Press OK { fout: geen corresponderende '}' gevonden }" when the message should have been "Press OK to delete all data". \$\endgroup\$
    – G. Sliepen
    Oct 30, 2021 at 19:10
  • \$\begingroup\$ I agree that throwing an exception on a format error is problematic in real life. Consider a Warning log message that isn't normally executed. A mistake there will turn the warning into a fatal error. And, it was not detected right away because the unit tests don't provide coverage for every single warning case. I've gone to some effort to re-wrap or alter formatting libraries to not have this behavior. \$\endgroup\$
    – JDługosz
    Nov 1, 2021 at 22:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.