Conversation
…::format, ensure it is a std::string or c-string
johnhaddon
left a comment
johnhaddon
left a comment
There was a problem hiding this comment.
Thanks Alex! What sort of timeframe are you hoping to get this merged in? As I've noted inline, I think it'd be better if we approached this with custom formatters in several cases, meaning some of the solution at least would be in Cortex. Cortex doesn't currently have a fmtlib dependency although I would love to replace boost::format with it throughout (as we've already done for Gaffer). Would the 1.7 timeframe be acceptable, so we can introduce the dependency in Cortex?
| if( !std::filesystem::is_regular_file( kernelFile ) ) | ||
| { | ||
| IECore::msg( IECore::Msg::Error, "IECoreCycles::init", fmt::format( "File \"{}\" not found", kernelFile ) ); | ||
| IECore::msg( IECore::Msg::Error, "IECoreCycles::init", fmt::format( "File \"{}\" not found", kernelFile.string() ) ); |
There was a problem hiding this comment.
I think we can #include "fmt/std.h" to get automatically formatting of paths.
| { | ||
| spec.channelnames.push_back( | ||
| fmt::format( "{}{}{}", layerName, layerName.size() ? "." : "", g_channels[i] ) | ||
| fmt::format( "{}{}{}", layerName, layerName.size() ? "." : "", g_channels[i].string() ) |
There was a problem hiding this comment.
I think we should have Cortex provide a formatter for InternedString, so we don't need to do this at all call sites.
| fmt::format( | ||
| "{} attribute edit{} required geometry to be regenerated", | ||
| m_failedAttributeEdits, m_failedAttributeEdits > 1 ? "s" : "" | ||
| m_failedAttributeEdits.load(), m_failedAttributeEdits > 1 ? "s" : "" |
There was a problem hiding this comment.
I think this is another one that would be dealt with by #include "fmt/std.h"
| if( !a ) | ||
| { | ||
| msg( Msg::Warning, messageContext, fmt::format( "Unable to create array from data of type \"{}\" for parameter \"{}\"", value->typeName(), name ) ); | ||
| msg( Msg::Warning, messageContext, fmt::format( "Unable to create array from data of type \"{}\" for parameter \"{}\"", value->typeName(), name.c_str() ) ); |
There was a problem hiding this comment.
There are enough of these that perhaps it would be better to register a formatter for AtString? I think format_as allows us to do that quite simply?
2 participants
Fmt : When using Fmt 10.x it is more strict about types passed to fmt::format, ensure it is a std::string or c-string
Generally describe what this PR will do, and why it is needed
Related issues
Dependencies
Breaking changes
Checklist