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

Check range_begin is dereferenceable #3964

Merged
merged 1 commit into from
May 20, 2024

Conversation

Arghnews
Copy link
Contributor

@Arghnews Arghnews commented May 19, 2024

Fixes issue #3839

An Eigen 3.4 2x2 matrix has a begin member function that returns void and a static_assert that evaluates to false (as you're not meant to call begin on a non-1d matrix).
However this doesn't play nicely with the SFINAE checks at the moment in fmt as they simply perform a SFINAE check that the expression T{}.begin() is valid, which evaluating to void is (even though it's of course not a valid type for an iterator):

fmt/include/fmt/ranges.h

Lines 100 to 104 in 75e8924

template <typename T>
struct has_const_begin_end<
T,
void_t<
decltype(detail::range_begin(std::declval<const remove_cvref_t<T>&>())),


To model a range more strictly to eliminate this case:

If we look at the c++20 concept for a range, it requires ranges::begin on the type, and that requires the iterator type satisfy input_or_output_iterator, which specifically notes that begin returns: The exposition-only concept /*can-reference*/ is satisfied if and only if the type is referenceable (in particular, not void).

So just check we can deref the result of that type to fix this case, and not incorrectly identify a type with void begin/end functions as a range. Note we don't want to do this for the type returned by end, as (in c++ >= 20) it may be a different sentinel type that is not dereferenceable

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! Overall looks good, just minor comments inline.

Comment on lines 752 to 762
FMT_BEGIN_NAMESPACE
template <> struct formatter<not_range> : formatter<string_view> {
auto format(const not_range&, format_context& ctx) const
-> format_context::iterator {
return formatter<string_view>::format("Dummy output", ctx);
}
};
FMT_END_NAMESPACE
TEST(ranges_test, format_not_range_using_specialized_formatter) {
EXPECT_EQ("Dummy output", fmt::format("{}", not_range{}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's sufficient to check that is_formattable<not_range> compiles and returns false. We don't need to introduce a formatter 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.

If you're satisfied with that, then I'll remove the test and this can just have the static assert then. AFAIK there isn't a gtest way to static assert so will just use a regular one

Copy link
Contributor

Choose a reason for hiding this comment

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

You can keep the test and make a dynamic check (e.g. EXPECT_FALSE) instead of static_assert. Then at least some of the failures might be recoverable. But it's not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had pushed before your comment, apologies, but seems like failing as early as possible ie. compile time rather than runtime is fine anyway, so will leave it without the test if you're ok with that, can change it back if not

test/ranges-test.cc Outdated Show resolved Hide resolved
Fixes issue fmtlib#3839
An Eigen 3.4 2x2 matrix has a begin member function that returns void
Be more strict checking that the result of calling *begin() is valid
See input_or_output_iterator concept notes about void
@vitaut vitaut merged commit b817610 into fmtlib:master May 20, 2024
40 checks passed
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