Skip to main content
added 23 characters in body
Source Link
user673679
  • 12.2k
  • 2
  • 34
  • 65
#include <cassert>
#include <charconv>
#include <iostream>
#include <regex>
#include <string>
#include <vector>

namespace detail
{

    std::string to_string(std::string s) { return s; }

    template<class T>
    std::string arg_to_string(T&& t)
    {
        using detail::to_string;
        using std::to_string;
        return to_string(std::forward<T>(t));
    }

    std::size_t convert_index(std::string::const_iterator begin, std::string::const_iterator end)
    {
        auto value = std::size_t{ 0 };
        auto result = std::from_chars(&*begin, &*end, value);

        assert(result.ptr == &*end);
        assert(result.ec == std::errc{ });

        return value;
    };

} // detail

template<typename... Args>
std::string format(std::string const& format_str, Args&&... args)
{
    auto const arguments = std::vector<stdarray<std::string>string, sizeof...(Args)>{ detail::arg_to_string(std::forward<Args>(args))... };

    auto arguments_total_size = std::size_t{ 0 };
    for (auto const& a : arguments)
        arguments_total_size += a.size();

    auto result = std::string();
    result.reserve(format_str.size() + arguments_total_size);

    auto begin = format_str.cbegin();
    auto const end = format_str.cend();
    auto const regex = std::regex("\\{(\\d+)\\}");

    while (true)
    {
        std::smatch match;
        if (!std::regex_search(begin, end, match, regex))
            break;

        result.append(match.prefix().first, match.prefix().second);

        auto index = detail::convert_index(match[1].first, match[1].second);
        result.append(arguments.at(index));

        begin = match.suffix().first;
    }

    result.append(begin, end);

    return result;
}

int main()
{
    std::cout << format("test: {0}, {1}, {0}, {3}", "5", 5, 123.f, std::string("test")) << std::endl;
}
#include <cassert>
#include <charconv>
#include <iostream>
#include <regex>
#include <string>
#include <vector>

namespace detail
{

    std::string to_string(std::string s) { return s; }

    template<class T>
    std::string arg_to_string(T&& t)
    {
        using detail::to_string;
        using std::to_string;
        return to_string(std::forward<T>(t));
    }

    std::size_t convert_index(std::string::const_iterator begin, std::string::const_iterator end)
    {
        auto value = std::size_t{ 0 };
        auto result = std::from_chars(&*begin, &*end, value);

        assert(result.ptr == &*end);
        assert(result.ec == std::errc{ });

        return value;
    };

} // detail

template<typename... Args>
std::string format(std::string format_str, Args&&... args)
{
    auto const arguments = std::vector<std::string>{ detail::arg_to_string(std::forward<Args>(args))... };

    auto arguments_total_size = std::size_t{ 0 };
    for (auto const& a : arguments)
        arguments_total_size += a.size();

    auto result = std::string();
    result.reserve(format_str.size() + arguments_total_size);

    auto begin = format_str.cbegin();
    auto const end = format_str.cend();
    auto const regex = std::regex("\\{(\\d+)\\}");

    while (true)
    {
        std::smatch match;
        if (!std::regex_search(begin, end, match, regex))
            break;

        result.append(match.prefix().first, match.prefix().second);

        auto index = detail::convert_index(match[1].first, match[1].second);
        result.append(arguments.at(index));

        begin = match.suffix().first;
    }

    result.append(begin, end);

    return result;
}

int main()
{
    std::cout << format("test: {0}, {1}, {0}, {3}", "5", 5, 123.f, std::string("test")) << std::endl;
}
#include <cassert>
#include <charconv>
#include <iostream>
#include <regex>
#include <string>
#include <vector>

namespace detail
{

    std::string to_string(std::string s) { return s; }

    template<class T>
    std::string arg_to_string(T&& t)
    {
        using detail::to_string;
        using std::to_string;
        return to_string(std::forward<T>(t));
    }

    std::size_t convert_index(std::string::const_iterator begin, std::string::const_iterator end)
    {
        auto value = std::size_t{ 0 };
        auto result = std::from_chars(&*begin, &*end, value);

        assert(result.ptr == &*end);
        assert(result.ec == std::errc{ });

        return value;
    };

} // detail

template<typename... Args>
std::string format(std::string const& format_str, Args&&... args)
{
    auto const arguments = std::array<std::string, sizeof...(Args)>{ detail::arg_to_string(std::forward<Args>(args))... };

    auto arguments_total_size = std::size_t{ 0 };
    for (auto const& a : arguments)
        arguments_total_size += a.size();

    auto result = std::string();
    result.reserve(format_str.size() + arguments_total_size);

    auto begin = format_str.cbegin();
    auto const end = format_str.cend();
    auto const regex = std::regex("\\{(\\d+)\\}");

    while (true)
    {
        std::smatch match;
        if (!std::regex_search(begin, end, match, regex))
            break;

        result.append(match.prefix().first, match.prefix().second);

        auto index = detail::convert_index(match[1].first, match[1].second);
        result.append(arguments.at(index));

        begin = match.suffix().first;
    }

    result.append(begin, end);

    return result;
}

int main()
{
    std::cout << format("test: {0}, {1}, {0}, {3}", "5", 5, 123.f, std::string("test")) << std::endl;
}
Source Link
user673679
  • 12.2k
  • 2
  • 34
  • 65

  • Making functions in header files static means that each translation unit gets its own separate definition. This usually isn't what we want. Standard practice is to declare the function in the header, and then define it in a .cpp file, e.g.:

     // header:
    
     #include <string>
    
     namespace lwlog
     {
         namespace datetime
         {
             std::string get_current_time_and_date(const char* format);
         }
     }
    

    .

     // .cpp:
    
     #include <iomanip>
     #include <sstream>
     #include <chrono>
     #include <ctime>
    
     namespace lwlog
     {
         namespace datetime
         {
             static std::string get_current_time_and_date(const char* format)
             {
                 auto now = std::chrono::system_clock::now();
                 auto in_time_t = std::chrono::system_clock::to_time_t(now);
    
                 std::stringstream ss;
                 ss << std::put_time(std::localtime(&in_time_t), format);
                 return ss.str();
             }
         }
     }
    

    While this is a pain in the butt, it means that anyone including datetime.h doesn't also get the sstream chrono, etc. headers as an unnecessary side-effect.


  • Prefer to declare variables as close to the point of use as possible, and (ideally) assign the necessary value directly, e.g. for the local variables in the print function.

  • Descriptive names are nice, but something like detail::populate_vec_with_variadic_params is probably overkill. Maybe detail::string_vec_from_args instead? Likewise with the local variables: variadic_arguments_vec -> arguments, format_string_tokens_vec -> format_tokens.


The various detail functions used to print with could be simplified quite a bit:

  • Rather than populate_vec_with_variadic_params we can define our own to_string function, and then do something like:

     namespace detail
     {
    
         std::string to_string(std::string s) { return s; }
    
         template<class T>
         std::string arg_to_string(T&& t)
         {
             using detail::to_string;
             using std::to_string;
             return to_string(std::forward<T>(t));
         }
    
     } // detail
    
     ...
    
         auto arguments = std::vector<std::string>{ detail::arg_to_string(std::forward<Args>(args))... };
    

    This has the advantage of allowing users to specify to_string functions for their own classes too.


  • populate_vec_with_regex_matches_from_str:

  • shouldn't take s by reference, as it doesn't change s (this allows us to eliminate the temp variable).

  • should simply return a vector, instead of altering one.

  • should use iterators to avoid unnecessary copies (both internally, and in the vector it returns).

  • rather than capturing the curly brackets and then removing them later, we can just capture the number inside the curly brackets.

  • we need to capture one or more digit inside the brackets, not a single digit character, or the function will stop working when we get to 10 arguments.

        using substring_view = std::pair<std::string::const_iterator, std::string::const_iterator>;
    
        std::vector<substring_view> find_matches(std::regex rg, std::string const& s)
        {
            auto result = std::vector<substring_view>();
    
            auto begin = s.cbegin();
            auto const end = s.cend();
    
            std::smatch match;
            while (std::regex_search(begin, end, match, rg))
            {
                result.push_back({ match[1].first, match[1].second });
                begin = match.suffix().first;
            }
    
            return result;
        }
    
        ...
    
            auto format_strings = detail::find_matches(std::regex("\\{(\\d+)\\}"), format_str);
    

  • remove_duplicates_in_vec:
  • can be a call to std::unique.
  • should be applied to format_strings_tokens_vec instead of variadic_arguments_vec!!! We want to remove duplicate indices, not duplicate arguments.

  • string_to_numeric_vec:

  • could use std::transform.

  • should produce indices of std::size_t (a.k.a. std::vector<T>::size_type), not int.

  • should probably do some sort of error handling on conversion failure (even just assertions).

  • std::from_chars is probably the best standard converter available for this.

        std::vector<std::size_t> convert_indices(std::vector<substring_view> const& s)
        {
            auto result = std::vector<std::size_t>(s.size(), 0);
    
            auto convert = [] (substring_view const& v)
            {
                auto const begin = &*v.first;
                auto end = &*v.second;
                auto value = std::size_t{ 0 };
                auto result = std::from_chars(begin, end, value);
    
                assert(result.ptr == end);
                assert(result.ec == std::errc{ });
    
                return value;
            };
    
            std::transform(s.begin(), s.end(), result.begin(), convert);
    
            return result;
        }
    

  • replace_in_string is called for every format token index, and iterates over the whole string each time, turning the complexity from O(n) to O(n^2). Yikes!

    We'll be doing much less work if we do the "replacement" at the same time as we do the regex search.

So I'd probably do something like this:

#include <cassert>
#include <charconv>
#include <iostream>
#include <regex>
#include <string>
#include <vector>

namespace detail
{

    std::string to_string(std::string s) { return s; }

    template<class T>
    std::string arg_to_string(T&& t)
    {
        using detail::to_string;
        using std::to_string;
        return to_string(std::forward<T>(t));
    }

    std::size_t convert_index(std::string::const_iterator begin, std::string::const_iterator end)
    {
        auto value = std::size_t{ 0 };
        auto result = std::from_chars(&*begin, &*end, value);

        assert(result.ptr == &*end);
        assert(result.ec == std::errc{ });

        return value;
    };

} // detail

template<typename... Args>
std::string format(std::string format_str, Args&&... args)
{
    auto const arguments = std::vector<std::string>{ detail::arg_to_string(std::forward<Args>(args))... };

    auto arguments_total_size = std::size_t{ 0 };
    for (auto const& a : arguments)
        arguments_total_size += a.size();

    auto result = std::string();
    result.reserve(format_str.size() + arguments_total_size);

    auto begin = format_str.cbegin();
    auto const end = format_str.cend();
    auto const regex = std::regex("\\{(\\d+)\\}");

    while (true)
    {
        std::smatch match;
        if (!std::regex_search(begin, end, match, regex))
            break;

        result.append(match.prefix().first, match.prefix().second);

        auto index = detail::convert_index(match[1].first, match[1].second);
        result.append(arguments.at(index));

        begin = match.suffix().first;
    }

    result.append(begin, end);

    return result;
}

int main()
{
    std::cout << format("test: {0}, {1}, {0}, {3}", "5", 5, 123.f, std::string("test")) << std::endl;
}

N.B. We could just send everything straight to std::cout instead of building the formatted string.