5
\$\begingroup\$

While upgrading some meta-program running in production for years to the latest (c++20) standard, I rewrote this particular compile time string utility.

Even though this new version produces desired output and correctly evaluates at compile time as well as dynamically, it's difficult to get a grasp on a new standard as always.

This example is sufficient for my particular use case. And for now I can only spot three inconsistencies I'd like to improve: two of them are marked in the code snippet with TODO, and the third one is tiding it up with concepts (like convertable_to, which is deliberately omitted in the code for simplicity sake)

I'm looking for a general case advice on how this code can be improved further. Any other weighted criticism will be much appreciated.

UPDATE: New question with all of the suggestions integrated

Original code:

Live @godbolt

#ifndef META_STRING_H_INCLUDED
#define META_STRING_H_INCLUDED

#include <cstddef>
#include <algorithm>
#include <functional>
#include <tuple>

namespace meta
{

template <std::size_t N>
struct string;

constexpr auto to_string(const auto& input) { return string(input); }

constexpr auto concat(const auto&... input) { return string(to_string(input)...); }

template <std::size_t N>
struct string
{
    char elems[N];

    // string() = delete;
    string() { elems[N - 1] = '\0'; } // used for CTAD guide for tuples. can't we avoid object construction there?

    constexpr string(const char (&s)[N])
    {
        std::copy_n(s, N, this->elems);
    }

    constexpr string(const string<N> (&s))
    {
        std::copy_n(s.elems, N, this->elems);
    }

    constexpr string(const std::array<char, N> (&s))
    {
        std::copy_n(s.data(), N, this->elems);
    }
    
    template <std::size_t... Ni>
    constexpr void _copy(const string<Ni> (&... input))
    {
        auto pos = elems;
        ((pos = std::copy_n(input.elems, Ni - 1, pos)), ...);
        *pos = 0;
    }

    constexpr string(const auto&... input) requires (sizeof...(input) > 1)
    {
        std::invoke([this](const auto&... s) constexpr { this->_copy(s...); }, to_string(input)...);
    }

    template <template <typename...> typename container, typename... T>
    constexpr string(const container<T...>& input)
    {
        std::apply([this](const auto&... s) constexpr { this->_copy(to_string(s)...); }, input);
    }

    constexpr auto operator + (const auto& rhs) const
    {
        return concat(*this, rhs);
    }

    constexpr operator const char* () const { return elems; }
};

template<std::size_t N> string(const char (&)[N])
    -> string<N>;

template<std::size_t N> string(const std::array<char, N>& input)
    -> string<N>;

string(const auto&... input)
    -> string<((sizeof(to_string(input).elems) - 1) + ... + 1)>;

template<template <typename...> typename container, typename... T> string(const container<T...>& input)
    -> string<((sizeof(to_string(T()).elems) - 1) + ... + 1)>; // TODO: avoid constructing object here


inline namespace meta_string_literals {

template<string ms>
inline constexpr auto operator"" _ms() noexcept
{
    return ms;
}

} // inline namespace meta_string_literals

} // namespace meta

#endif // META_STRING_H_INCLUDED

//////////////////////////////////////////////////////////////////////

// #include "meta_string.h"
#include <iostream>

template<meta::string str>
struct X
{
    static constexpr auto value = str;
    operator const char* () { return str.elems; }
};

template <auto value>
constexpr inline auto constant = value;

int main()
{
    using namespace meta::meta_string_literals;


    X<"a message"> xxx;
    X<"a massage"> yyy;
    X<meta::concat(xxx.value, " is not ", yyy.value)> zzz;
    X<"a message"_ms + " is " + "a massage"> zzz2;

    std::cout << xxx << std::endl;
    std::cout << yyy << std::endl;
    std::cout << zzz << std::endl;
    std::cout << zzz2 << std::endl;


    static constexpr auto x = meta::string("1"_ms, "22");
    static constexpr auto y = meta::concat("11", "22");
    static constexpr auto z = meta::string(std::tuple{"1xx1"_ms, std::array<char, 6>{"2qqq2"}});

    std::cout << sizeof(x.elems) << ": " << x << std::endl;
    std::cout << sizeof(y.elems) << ": " << y << std::endl;
    std::cout << sizeof(z.elems) << ": " << z << std::endl;

    static constexpr auto a = "1"_ms;
    static constexpr auto b = a + "22"_ms;
    std::cout << b << std::endl;

    // TODO: Can't it be implicitly forced to constexpr?
    std::cout << meta::string("this one "_ms, "is not ", "constant evaluated"_ms) << std::endl;
    std::cout << constant<meta::string("this one "_ms, "is ", "constant evaluated"_ms)> << std::endl;

    return 0;
}
\$\endgroup\$
2
  • \$\begingroup\$ You might want this to convert to a std::string_view, as well as char*. It’s becoming common to accept std::string_view as the common denominator of all string formats. \$\endgroup\$
    – Davislor
    Commented Dec 24, 2021 at 1:08
  • \$\begingroup\$ I find the name confusing. A fixed-buffer string is not a "meta string". Also, I find it especially handy to have such a thing that implicitly converts to string_view, in implementing things like a variadic concatenate-everything function, that avoids heap allocation. \$\endgroup\$
    – JDługosz
    Commented Dec 24, 2021 at 7:41

2 Answers 2

1
\$\begingroup\$

This looks more messy than I would prefer, partly because of limitations in C++, partly because you have a constructor that takes a container as an argument. So I don't see another way to do it, just small things that could be improved here and there.

Unnecessary use of this->

It is almost never necessary to write this-> in C++. I would remove its use everywhere.

to_string() is not necessary

I see you have a free function to_string() to ensure you can cast something to a string with a different N than is used in the templated code you are in, and also to "escape" the deduction guides. This is not a bad approach, but it might not be necessary to use it.

Inside class string you can avoid calling to_string(input) by writing meta::string(input).

In the deduction guides it is harder to work around this, you can't just write meta::string(input).elems. But see below for a possible solution that also gets rid of the default constructor.

I would be OK with leaving it as you wrote it though, but if it is only intended to be used as a helper function for struct string, consider wrapping it inside a namespace detail to signal it should not be used by external code directly.

Unnecessary use of std::invoke()

You don't need std::invoke() at all, the constructor that uses it can just be rewritten as:

constexpr string(const auto&... input) requires (sizeof...(input) > 1)
{
    _copy(to_string(input)...);
}

Make _copy() private

The member function _copy() is just a helper function used by the constructors, so it should be made private. I would also remove the leading underscore from the name of this function, as some uses of leading and double underscores in identifiers are reserved, and the simplest way to avoid any issues is to avoid all leading and double underscores.

Consider adding size() and other const member functions from std::string

I noticed the sizeof(to_string(input).elems) in the deduction guides, and I thought that looked a bit awkward; if there was a size() member function it would be nicer to be able to write to_string(input).size(). Unfortunately, even with a static constexpr size() function, this is not considered a constant expression, so that won't compile. But it will make some of your example usage cleaner:

meta::string x(...);
std::cout << x.size() << ": " << x << '\n';

You might consider other const member functions from std::string as well, like empty(), data(), operator[] and so on, to make it a better drop-in replacement for const std::strings.

Deleting the default constructor

To be able to delete the default constructor, you need to avoid default constructing meta::strings. That means you can't use to_string(T()) if T is a meta::string. There is a very simple solution here, just use sizeof(T):

template<template <typename...> typename container, typename... T> string(const container<T...>& input)
    -> string<((sizeof(T) - 1) + ... + 1)>;

The drawback, as user17732522 mentioned, is that it doesn't seem to work when constructing a meta::string with nested std::tuples.

Forcing constant evaluation of rvalue

I don't think this is possible. At the same time I don't understand why the compiler wouldn't be able to optimize this at compile time, even without using constexpr.

Avoid using std::endl

Prefer using \n instead of std::endl; the latter is equivalent to the former, but also forces the output to be flushed, which is usually unnecessary and hurts performance.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ this-> isn't required in the lambda either. \$\endgroup\$ Commented Dec 25, 2021 at 14:31
  • 1
    \$\begingroup\$ sizeof(T) is probably not correct in all situations. It is not entire clear what types are supposed to be allowed, but it could for example be another std::tuple. \$\endgroup\$ Commented Dec 25, 2021 at 14:38
  • 1
    \$\begingroup\$ Personally I was impressed by how @G. Sliepen spotted this-> usage inside of a lambda. Testing out different compilers/versions/flags(-O, -pedantic, e.t.c.) I stumbled into many edge cases, and found out that's the only way to make it always ("cross")compile is to use this-> inside of a lambda. In general I don't mind a little more verbosity here and there (like this-> inside constructors) if it imposes a bit more readability. Remember 20 years back, during the MFC era, one might write m_elems. \$\endgroup\$
    – Dmitrii
    Commented Dec 26, 2021 at 14:46
  • \$\begingroup\$ sizeof(T) is the one particular case I'm trying to avoid at any cost, due to being very cautious with alignment, padding and "structure packing". It can drive us into classical shoot yourself problem very easily. If we can avoid it using meta-programming techniques we should. I will append a modified version of the code with all of the suggestions integrated soon. As imo @G. Sliepen's answer is clearly a solid and throughout analysis and should be accepted. \$\endgroup\$
    – Dmitrii
    Commented Dec 26, 2021 at 15:01
  • \$\begingroup\$ As for sizeof(T), I don't think it is a problem for any of the types you are able to support at compile time, but I might be wrong. I strongly suggest you post the modified version of the code as a new question on Code Review, so it can be reviewed on its own. You can add a link to the new question in this post. \$\endgroup\$
    – G. Sliepen
    Commented Dec 26, 2021 at 19:11
2
\$\begingroup\$
  • We are in unevaluated context here.

    // TODO: avoid constructing object here
    template<template <typename...> typename container, typename... Ts>
    string(const container<Ts...>& input)
    -> string<((sizeof(to_string(Ts()).elems) - 1) + ... + 1)>;
    

    You can replace Ts() by std::declval<Ts>().

    Demo

  • // TODO: Can't it be implicitly forced to constexpr?
    std::cout << meta::string("this one "_ms, "is not ", "constant evaluated"_ms) << std::endl;
    

    constexpr is evaluated at compile-time only in constant expression. Optimization can do its jobs though (and constexpr might be a good hint).

    By applying consteval, you might got an error here.

\$\endgroup\$

Your Answer

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

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