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.

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

Add support for ranges, containers and types tuple interface... #735

Closed
wants to merge 13 commits into from
Closed

Add support for ranges, containers and types tuple interface... #735

wants to merge 13 commits into from

Conversation

Remotion
Copy link
Contributor

No description provided.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good, but please follow the style guidelines described ]here, particularly 2-space indent (I think you can use clang-format with Google Style settings to fix this automatically). Also a few comments inline.

template<typename OutputIterator>
void copy(const char ch, OutputIterator out) {
*out++ = ch;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest putting all of the above in the internal namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

template<typename OutputIterator>
void copy(const char ch, OutputIterator out) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const is not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done



namespace fmt {
namespace meta {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using internal namespace instead of meta here for consistency with the rest of the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

template<typename T>
class is_like_std_string {
template<typename U> static auto check(U* p) -> decltype(
p->find('a')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to require find for string-like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is length() and data() really enough to be sure that this is std::string like type?

};

template<typename T>
constexpr bool is_like_std_string_v = is_like_std_string<T>::value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere: constexpr -> FMT_CONSTEXPR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Char prefix = '{';
Char delimiter = ',';
Char postfix = '}';
bool add_spaces = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Containers are usually formatted without embedded spaces, in particular in Python format which this library is modeled after:

>>> '{}'.format([1, 2])
'[1, 2]'

so I suggest dropping this flag (or at least setting it to false).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, set add_spaces to false per default.

template <typename ParseContext>
FMT_CONSTEXPR auto parse(ParseContext &ctx) -> decltype(ctx.begin()) {
return formating.parse(ctx);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be needed if you inherit from formatting_tuple.

template <typename FormatContext = format_context>
auto format(const TupleT &values, FormatContext &ctx) -> decltype(ctx.out()) {
auto out = ctx.out();
std::ptrdiff_t i = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, replaced by size_t.
I needed signed type in older implementation but now it can be size_t.




namespace fmt {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be merged with the above namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

template<typename RangeT, typename Char>
struct formatter <RangeT, Char, std::enable_if_t<fmt::meta::is_range_v<RangeT>> >
{
static constexpr std::ptrdiff_t range_length_limit = FMT_RANGE_OUTPUT_LENGTH_LIMIT; // output only up to N items from the range.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, ditto.

vitaut and others added 3 commits May 12, 2018 13:23
Renames namespace meta to internal and put copy functions int it.
Replaced constexpr by FMT_CONSTEXPR.
Replaced std::ptrdiff_t by std::size_t.
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Just a few more comments/questions.

struct formatting_tuple : formatting_base<Char> {
Char prefix = '[';
Char delimiter = ',';
Char postfix = ']';
Copy link
Contributor

Choose a reason for hiding this comment

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

Tuples are normally delimited by parentheses () rather than square brackets [].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using () in last commit now.

internal::copy(formatting.prefix, out);
internal::for_each(values, [&](const auto &v) {
if (i++ > 0) {
internal::copy(formatting.delimiter, out);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding a space after delimiter as in (1, 2).

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 can be configured now by bool add_delimiter_spaces

typename std::enable_if<fmt::internal::is_range<RangeT>::value>::type> {

static FMT_CONSTEXPR_DECL const std::size_t range_length_limit =
FMT_RANGE_OUTPUT_LENGTH_LIMIT; // output only up to N items from the range.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a limit? User can always pass a subrange to fmt::join if they don't want to format everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it could be and infinite range, or nested range where user can not use fmt::join.
std::tuple<std::vector<std::vector<int>>>

}

#if (__cplusplus > 201402L) || \
(defined(_MSVC_LANG) && _MSVC_LANG > 201402L && _MSC_VER >= 1910)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to check if constexpr is supported?

Copy link
Contributor Author

@Remotion Remotion May 13, 2018

Choose a reason for hiding this comment

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

This is to check if if constexpr is supported.

// {fmt} support for ranges, containers and types tuple interface.

#ifdef WIN32
#define _CRT_SECURE_NO_WARNINGS
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

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 can be removed.

@vitaut
Copy link
Contributor

vitaut commented May 13, 2018

Merged with minor formatting tweaks in e3f7f3a. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants