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

fix: support optional<T> with format_as(T) (#3712) #3713

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

cptFracassa
Copy link
Contributor

@cptFracassa cptFracassa commented Nov 13, 2023

Formatting a std::optional<T> where T had a custom format_as(T) function failed to compile with clang, due to set_debug_format being hidden by private inheritance. This fix makes the function available through a using clause.

This fixes #3712

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 but could you add a test case to https://github.com/fmtlib/fmt/blob/master/test/std-test.cc?

@cptFracassa
Copy link
Contributor Author

cptFracassa commented Nov 15, 2023

Thanks for the PR. Overall looks good but could you add a test case to https://github.com/fmtlib/fmt/blob/master/test/std-test.cc?

I have done so now 😃
The lines 113 and 116 of std-test.cc both fails to compile if the new line in format.h is commented out when using clang.

test/std-test.cc Outdated
one,
two,
};
auto format_as(my_number number) {
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 below a return type is 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.

Here and below a return type is needed.
Added now. I am spoiled by working with C++20...

@@ -90,6 +90,34 @@ TEST(std_test, optional) {
#endif
}

namespace my_nso {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "nso" stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does "nso" stand for?

I was trying to follow the naming scheme with short namespaces below, my_ns1 & my_ns2, and added an o for optional, since my_ns3 before the two others didn't make sense. But I'll change to whatever you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

my_nso is OK

Formatting a std::optional<T> where T had a custom format_as(T) function failed to compile with clang,
due to set_debug_format being hidden by private inheritance. This fix makes the function available through a using clause.
@vitaut vitaut merged commit 864a8b5 into fmtlib:master Nov 16, 2023
40 checks passed
happymonkey1 pushed a commit to happymonkey1/fmt that referenced this pull request Apr 7, 2024
Formatting a std::optional<T> where T had a custom format_as(T) function failed to compile with clang,
due to set_debug_format being hidden by private inheritance. This fix makes the function available through a using clause.
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.

formatting a std::optional with format_as function fails to compile using clang
2 participants