Build against more recent versions of {fmt} #2639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fpsunflower
reviewed
Jul 10, 2020
| template <> | ||
| struct formatter<OIIO::TypeDesc> { | ||
| // Parses format specification | ||
| // C++14: constexpr auto parse(format_parse_context& ctx) { |
There was a problem hiding this comment.
Did you mean to add contexpr here in c++14 mode ?
There was a problem hiding this comment.
No, I just meant for the comment to remind myself that when C++14 is our minimum, I can switch to the more compact declaration. I barely noticed the constexpr part, the big difference was that C++11 needs the "-> decltype(...)" to go along with the "auto", but C++14 can deduce the return type.
There was a problem hiding this comment.
I'm assuming that constexpr is desirable for when you want to use the compile time format string parsing feature - but I guess we are not using this widely right now?
There was a problem hiding this comment.
Not using it at all for the moment.
lgritz
commented
Jul 10, 2020
fpsunflower
approved these changes
Jul 10, 2020
fmt is supposed to (if you include fmt/ostream.h) automatically
support any user type that defines an `operator<<()`. And so it did
through version 6.1.2.
Starting with 6.2.x, the presence of `operator bool()` throws it off,
particularly for our TypeDesc, which started rendering as "true" rather
than "uint8" or whatever. It seems that the fact that it can convert
to bool takes precedence, it formats as a bool, rather than notice that
there's a stream ouput option.
In this patch, we add a custom formatter for TypeDesc, which solves
the problem for `format("{}", typedesc)`, but unfortunately not for
`sprintf("%s", typedesc)`. I've filed an issue, but in any case, the
workaround to avoid incorrect formatting when using sprintf is to be
sure to use typedesc.c_str().
Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jul 11, 2020
…n#2639) fmt is supposed to (if you include fmt/ostream.h) automatically support any user type that defines an `operator<<()`. And so it did through version 6.1.2. Starting with 6.2.x, the presence of `operator bool()` throws it off, particularly for our TypeDesc, which started rendering as "true" rather than "uint8" or whatever. It seems that the fact that it can convert to bool takes precedence, it formats as a bool, rather than notice that there's a stream ouput option. In this patch, we add a custom formatter for TypeDesc, which solves the problem for `format("{}", typedesc)`, but unfortunately not for `sprintf("%s", typedesc)`. I've filed an issue, but in any case, the workaround to avoid incorrect formatting when using sprintf is to be sure to use typedesc.c_str(). Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jul 14, 2020
…n#2639) fmt is supposed to (if you include fmt/ostream.h) automatically support any user type that defines an `operator<<()`. And so it did through version 6.1.2. Starting with 6.2.x, the presence of `operator bool()` throws it off, particularly for our TypeDesc, which started rendering as "true" rather than "uint8" or whatever. It seems that the fact that it can convert to bool takes precedence, it formats as a bool, rather than notice that there's a stream ouput option. In this patch, we add a custom formatter for TypeDesc, which solves the problem for `format("{}", typedesc)`, but unfortunately not for `sprintf("%s", typedesc)`. I've filed an issue, but in any case, the workaround to avoid incorrect formatting when using sprintf is to be sure to use typedesc.c_str(). Signed-off-by: Larry Gritz <lg@larrygritz.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fmt is supposed to (if you include fmt/ostream.h) automatically
support any user type that defines an
operator<<(). And so it didthrough version 6.1.2.
Starting with 6.2.x, the presence of
operator bool()throws it off,particularly for our TypeDesc, which started rendering as "true" rather
than "uint8" or whatever. It seems that the fact that it can convert
to bool takes precedence, it formats as a bool, rather than notice that
there's a stream ouput option.
In this patch, we add a custom formatter for TypeDesc, which solves
the problem for
format("{}", typedesc), but unfortunately not forsprintf("%s", typedesc). I've filed an issue, but in any case, theworkaround to avoid incorrect formatting when using sprintf is to be
sure to use typedesc.c_str().
Signed-off-by: Larry Gritz lg@larrygritz.com