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

Support printing (const) volatile void* #4056

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

Arghnews
Copy link
Contributor

@Arghnews Arghnews commented Jul 7, 2024

Fixes #4049

@Arghnews Arghnews force-pushed the volatile_void_pointer_support branch from 13860e8 to 63fb57c Compare July 7, 2024 21:18
Comment on lines 429 to 440
auto check = [](const int val) {
CHECK_ARG(
char, reinterpret_cast<const void*>(val),
static_cast<volatile void*>(reinterpret_cast<volatile int*>(val)));
CHECK_ARG(char, reinterpret_cast<const void*>(val),
static_cast<const volatile void*>(
reinterpret_cast<const volatile int*>(val)));
};
check(0);
check(1);
check(2);
check(0xbeef);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to check various values here, let's get rid of the lambda and just check nullptr.

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 was just being safe to cover the behaviour with iostreams linked in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1147r1.html but if you're happy for it to be removed I'll take it out

@Arghnews Arghnews force-pushed the volatile_void_pointer_support branch from 63fb57c to cc5e38f Compare July 9, 2024 20:04
@Arghnews Arghnews force-pushed the volatile_void_pointer_support branch from cc5e38f to 5e7b6a4 Compare July 9, 2024 20:21
@vitaut vitaut merged commit 13038f3 into fmtlib:master Jul 10, 2024
43 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jul 10, 2024

Merged, thanks!

@ZXShady
Copy link

ZXShady commented Jul 11, 2024

why 4 overloads instead of 1 overload taking a const volatile void*?

@Arghnews
Copy link
Contributor Author

why 4 overloads instead of 1 overload taking a const volatile void*?

It's a good question.

If all we had was 4 overloaded functions (void*, const void*, volatile void*, const volatile void*), I believe your suggestion would be fine.

However, there's also this:

fmt/include/fmt/base.h

Lines 1487 to 1498 in 6a192f8

// Use SFINAE instead of a const T* parameter to avoid a conflict with the
// array overload.
template <
typename T,
FMT_ENABLE_IF(
std::is_pointer<T>::value || std::is_member_pointer<T>::value ||
std::is_function<typename std::remove_pointer<T>::type>::value ||
(std::is_array<T>::value &&
!std::is_convertible<T, const char_type*>::value))>
FMT_CONSTEXPR auto map(const T&) -> unformattable_pointer {
return {};
}

as we only want to format (const)? (volatile)? void pointers, so this catches other pointer types to render them unformattable.

If we remove the first 3 of those, leaving only the const volatile void* overload, and we pass a void* - which should be formattable - the compiler will no longer find a function with a parameter that is an exact match.

It will then go and instantiate the unformattable function template above as the next best match. Our const volatile void* overload is lower precedence as it involves a conversion, and the function template just binds a const ref (is my understanding), which is not what we want.

See https://godbolt.org/z/3cTj5xErM and flip the define

You could probably get this working with some SFINAE (although I think you'll have a similar issue with getting int* to be unformattable still so you'll need another SFINAE function), but IMO overloading is generally simpler and easier to reason about, so preferred here

@ZXShady
Copy link

ZXShady commented Jul 13, 2024

@Arghnews

making the SFINAE condition

std::is_pointer<T>::value && !std::is_void<typename std::remove_pointer<T>::type>::value

will work.

I agree that overloading is easier to reason about. but I just wanted to point it out.

@Arghnews
Copy link
Contributor Author

@ZXShady Good point, the SFINAE isn't so bad actually.
We agree to prefer the overloads regardless.

Also I've learnt something from your comment, as I didn't know realise that all types of void* can bind to const volatile void*, so thank you

mtremer pushed a commit to ipfire/ipfire-2.x that referenced this pull request Aug 15, 2024
- Update from version 11.0.1 to 11.0.2
- Update of rootfile
- Changelog
    11.0.2
	- Fixed compatibility with non-POSIX systems
	  (fmtlib/fmt#4054,
	  fmtlib/fmt#4060).
	- Fixed performance regressions when using `std::back_insert_iterator` with
	  `fmt::format_to` (fmtlib/fmt#4070).
	- Fixed handling of `std::generator` and move-only iterators
	  (fmtlib/fmt#4053,
	  fmtlib/fmt#4057). Thanks @Arghnews.
	- Made `formatter<std::string_view>::parse` work with types convertible to
	  `std::string_view` (fmtlib/fmt#4036,
	  fmtlib/fmt#4055). Thanks @Arghnews.
	- Made `volatile void*` formattable
	  (fmtlib/fmt#4049,
	  fmtlib/fmt#4056). Thanks @Arghnews.
	- Made `Glib::ustring` not be confused with `std::string`
	  (fmtlib/fmt#4052).
	- Made `fmt::context` iterator compatible with STL algorithms that rely on
	  iterator category (fmtlib/fmt#4079).

Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
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.

can't print volatile void*
3 participants