Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/complete module #2340

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Feature/complete module #2340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b63ce15
Support compile-time strings and compile-time format string compilati…
DanielaE May 29, 2021
1a351a2
Support non-`char` overloads (module)
DanielaE Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions include/fmt/compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ struct is_compiled_string : std::is_base_of<compiled_string, S> {};
#endif

#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <typename Char, size_t N, fixed_string<Char, N> Str>
template <typename Char, size_t N,
fmt::detail_exported::fixed_string<Char, N> Str>
struct udl_compiled_string : compiled_string {
using char_type = Char;
constexpr operator basic_string_view<char_type>() const {
Expand Down Expand Up @@ -622,7 +623,7 @@ void print(const S& format_str, const Args&... args) {

#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
inline namespace literals {
template <detail::fixed_string Str>
template <detail_exported::fixed_string Str>
constexpr detail::udl_compiled_string<
remove_cvref_t<decltype(Str.data[0])>,
sizeof(Str.data) / sizeof(decltype(Str.data[0])), Str>
Expand Down
87 changes: 54 additions & 33 deletions include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,33 @@ FMT_INLINE auto make_args_checked(const S& fmt,
return {args...};
}

// compile-time support
namespace detail_exported {
#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <typename Char, size_t N> struct fixed_string {
constexpr fixed_string(const Char (&str)[N]) {
detail::copy_str<Char, const Char*, Char*>(static_cast<const Char*>(str),
str + N, data);
}
Char data[N]{};
};
#endif

// Converts a compile-time string to basic_string_view.
template <typename Char, size_t N>
constexpr auto compile_string_to_view(const Char (&s)[N])
-> basic_string_view<Char> {
// Remove trailing NUL character if needed. Won't be present if this is used
// with a raw character array (i.e. not defined as a string).
return {s, N - (std::char_traits<Char>::to_int_type(s[N - 1]) == 0 ? 1 : 0)};
}
template <typename Char>
constexpr auto compile_string_to_view(detail::std_string_view<Char> s)
-> basic_string_view<Char> {
return {s.data(), s.size()};
}
} // namespace detail_exported

FMT_BEGIN_DETAIL_NAMESPACE

inline void throw_format_error(const char* message) {
Expand Down Expand Up @@ -2098,20 +2125,6 @@ FMT_CONSTEXPR void handle_dynamic_spec(int& value,
}
}

// Converts a compile-time string to basic_string_view.
template <typename Char, size_t N>
constexpr auto compile_string_to_view(const Char (&s)[N])
-> basic_string_view<Char> {
// Remove trailing NUL character if needed. Won't be present if this is used
// with a raw character array (i.e. not defined as a string).
return {s, N - (std::char_traits<Char>::to_int_type(s[N - 1]) == 0 ? 1 : 0)};
}
template <typename Char>
constexpr auto compile_string_to_view(std_string_view<Char> s)
-> basic_string_view<Char> {
return {s.data(), s.size()};
}

#define FMT_STRING_IMPL(s, base, explicit) \
[] { \
/* Use the hidden visibility as a workaround for a GCC bug (#1973). */ \
Expand All @@ -2120,7 +2133,7 @@ constexpr auto compile_string_to_view(std_string_view<Char> s)
using char_type = fmt::remove_cvref_t<decltype(s[0])>; \
FMT_MAYBE_UNUSED FMT_CONSTEXPR explicit \
operator fmt::basic_string_view<char_type>() const { \
return fmt::detail::compile_string_to_view<char_type>(s); \
return fmt::detail_exported::compile_string_to_view<char_type>(s); \
} \
}; \
return FMT_COMPILE_STRING(); \
Expand All @@ -2138,16 +2151,6 @@ constexpr auto compile_string_to_view(std_string_view<Char> s)
*/
#define FMT_STRING(s) FMT_STRING_IMPL(s, fmt::compile_string, )

#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <typename Char, size_t N> struct fixed_string {
constexpr fixed_string(const Char (&str)[N]) {
copy_str<Char, const Char*, Char*>(static_cast<const Char*>(str), str + N,
data);
}
Char data[N]{};
};
#endif

#if FMT_USE_USER_DEFINED_LITERALS
template <typename Char> struct udl_formatter {
basic_string_view<Char> str;
Expand All @@ -2159,22 +2162,27 @@ template <typename Char> struct udl_formatter {
};

# if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <typename T, typename Char, size_t N, fixed_string<Char, N> Str>
template <typename T, typename Char, size_t N,
fmt::detail_exported::fixed_string<Char, N> Str>
struct statically_named_arg : view {
static constexpr auto name = Str.data;

const T& value;
statically_named_arg(const T& v) : value(v) {}
};

template <typename T, typename Char, size_t N, fixed_string<Char, N> Str>
template <typename T, typename Char, size_t N,
fmt::detail_exported::fixed_string<Char, N> Str>
struct is_named_arg<statically_named_arg<T, Char, N, Str>> : std::true_type {};

template <typename T, typename Char, size_t N, fixed_string<Char, N> Str>
template <typename T, typename Char, size_t N,
fmt::detail_exported::fixed_string<Char, N> Str>
struct is_statically_named_arg<statically_named_arg<T, Char, N, Str>>
: std::true_type {};

template <typename Char, size_t N, fixed_string<Char, N> Str> struct udl_arg {
template <typename Char, size_t N,
fmt::detail_exported::fixed_string<Char, N> Str>
struct udl_arg {
template <typename T> auto operator=(T&& value) const {
return statically_named_arg<T, Char, N, Str>(std::forward<T>(value));
}
Expand Down Expand Up @@ -2616,6 +2624,20 @@ template <typename Char>
void vformat_to(buffer<Char>& buf, basic_string_view<Char> fmt,
basic_format_args<buffer_context<type_identity_t<Char>>> args,
locale_ref loc) {
// workaround for msvc bug regarding name-lookup in module
// link names into function scope
using detail::arg_formatter;
using detail::buffer_appender;
using detail::custom_formatter;
using detail::default_arg_formatter;
using detail::get_arg;
using detail::locale_ref;
using detail::parse_format_specs;
using detail::specs_checker;
using detail::specs_handler;
using detail::to_unsigned;
using detail::type;
using detail::write;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure, if this is really needed by the standard or if this is just an implementation deficit. At least it looks ugly.

From reading "C++ Templates - The Definitive Guide" and the standard, it is clear that the local class is a templated entity. I need further studies of the standard to find an answer to the question: which names within this so-called 'temploid' are bound at which time.

I'd rather prefer to make this 'temploid' a proper class template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which local class are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct format_handler within vformat_to in format.h, currently line 2649.

The problem is explored with this stripped-down code example here https://godbolt.org/z/34Ph9dee9. Fortunately I got this compiled with gcc (modules-branch) for the first time just a few minutes ago, so that I have a second opinion on the matter now: gcc doesn't require this 'symbolic-linking' of names into the body of vformat_to and all of my studies of the standard seem to corroborate that. Therefore I consider this an implementation bug in latest msvc. This symbolic linking is the only workaround that works at all. Qualified name lookup doesn't. And only local classes within function templates are affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is acknowledged as bug in msvc by realiable sources familiar with the compiler: name-binding is happening in phase 2 instead of phase 1 in this particular case. In other words: too late to find anything in namespace detail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a short comment clarifying that this is a workaround for an msvc bug?

auto out = buffer_appender<Char>(buf);
if (fmt.size() == 2 && equal2(fmt.data(), "{}")) {
auto arg = args.get(0);
Expand Down Expand Up @@ -2672,13 +2694,12 @@ void vformat_to(buffer<Char>& buf, basic_string_view<Char> fmt,
begin = parse_format_specs(begin, end, handler);
if (begin == end || *begin != '}')
on_error("missing '}' in format string");
auto f =
detail::arg_formatter<Char>{context.out(), specs, context.locale()};
auto f = arg_formatter<Char>{context.out(), specs, context.locale()};
context.advance_to(visit_format_arg(f, arg));
return begin;
}
};
parse_format_string<false>(fmt, format_handler(out, fmt, args, loc));
detail::parse_format_string<false>(fmt, format_handler(out, fmt, args, loc));
}

#ifndef FMT_HEADER_ONLY
Expand Down Expand Up @@ -2721,7 +2742,7 @@ inline namespace literals {
\endrst
*/
#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <detail::fixed_string Str>
template <detail_exported::fixed_string Str>
constexpr auto operator""_a()
-> detail::udl_arg<remove_cvref_t<decltype(Str.data[0])>,
sizeof(Str.data) / sizeof(decltype(Str.data[0])), Str> {
Expand Down
8 changes: 4 additions & 4 deletions src/fmt.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
module;
#ifndef __cpp_modules
# error Module not supported.
#endif

// put all implementation-provided headers into the global module fragment
// to prevent attachment to this module
#if !defined(_CRT_SECURE_NO_WARNINGS) && defined(_MSC_VER)
Expand Down Expand Up @@ -76,10 +80,6 @@ export module fmt;
} \
export {

#if defined(_MSC_FULL_VER) && _MSC_FULL_VER > 192930036
#define FMT_USE_NONTYPE_TEMPLATE_PARAMETERS 0
#endif

// all library-provided declarations and definitions
// must be in the module purview to be exported
#include "fmt/args.h"
Expand Down
Loading