From: =?utf-8?q?B=C3=A5rd_Skaflestad?= Date: Tue, 17 Dec 2024 12:59:03 +0100 Subject: Abide by FMT-v11 Format String Requirements In particular, do not mix automatic formatting requests (empty braces, {}) with manual formatting requests (argument IDs and/or named arguments). The std::format() function of C++20 has the same limitation, so it's beneficial to prepare for this now. While here, prefer '//'-style comments, tidy up include statements and make objects 'const' where possible. --- opm/common/utility/OpmInputError.cpp | 94 +++++++++++--------- opm/common/utility/OpmInputError.hpp | 138 +++++++++++++++--------------- opm/input/eclipse/Parser/Parser.cpp | 22 +++-- tests/parser/integration/ParseKEYWORD.cpp | 25 +++--- 4 files changed, 150 insertions(+), 129 deletions(-) diff --git a/opm/common/utility/OpmInputError.cpp b/opm/common/utility/OpmInputError.cpp index 6a0c157..c52e02b 100644 --- a/opm/common/utility/OpmInputError.cpp +++ b/opm/common/utility/OpmInputError.cpp @@ -17,71 +17,85 @@ along with OPM. If not, see . */ +#include + #include #include +#include +#include +#include #include +#include #include -#include - -namespace Opm { namespace { -template -std::string formatImpl(const std::string& msg_format, const KeywordLocation& loc, const Args& ...arguments) { - return fmt::format(msg_format, - arguments..., - fmt::arg("keyword", loc.keyword), - fmt::arg("file", loc.filename), - fmt::arg("line", loc.lineno) - ); +template +std::string formatImpl(std::string_view msg_format, + const Opm::KeywordLocation& loc, + Args&&... arguments) +{ + return fmt::format(fmt::runtime(msg_format), + std::forward(arguments)..., + fmt::arg("keyword", loc.keyword), + fmt::arg("file", loc.filename), + fmt::arg("line", loc.lineno)); } -} +} // Anonymous namespace -std::string OpmInputError::formatException(const std::exception& e, const KeywordLocation& loc) { - const std::string defaultMessage { R"(Problem with keyword {keyword} +std::string +Opm::OpmInputError::formatException(const std::exception& e, + const KeywordLocation& loc) +{ + return formatImpl(R"(Problem with keyword {keyword} In {file} line {line}. -{})" } ; - - return formatImpl(defaultMessage, loc, e.what()); +{0})", + loc, e.what()); } -/* - For the format() function it is possible to have an alternative function with - a variaditic template which can be forwarded directly to the fmt::format() - function, that is an elegant way to pass arbitrary additional arguments. That - will require the OpmInputError::format() to become a templated function and - the fmtlib dependendcy will be imposed on downstream modules. -*/ -std::string OpmInputError::format(const std::string& msg_format, const KeywordLocation& loc) { +// Note: fmt::format() is a variadic template whence it is possible to +// forward arbitrary arguments directly to this function. On the other +// hand, using that ability will make OpmInputError::format() a function +// template in a header too and, moreover, confer the fmtlib prerequiste +// onto downstream modules. +std::string +Opm::OpmInputError::format(const std::string& msg_format, + const KeywordLocation& loc) +{ return formatImpl(msg_format, loc); } -std::string OpmInputError::formatSingle(const std::string& reason, const KeywordLocation& location) { - const std::string defaultMessage { R"(Problem with keyword {keyword} +std::string +Opm::OpmInputError::formatSingle(const std::string& reason, + const KeywordLocation& location) +{ + return formatImpl(R"(Problem with keyword {keyword} In {file} line {line} -{})" } ; - - return formatImpl(defaultMessage, location, reason); +{0})", + location, reason); } namespace { -std::string locationStringLine(const KeywordLocation& loc) { - return OpmInputError::format("\n {keyword} in {file}, line {line}", loc); +std::string locationStringLine(const Opm::KeywordLocation& loc) +{ + return Opm::OpmInputError::format("\n {keyword} in {file}, line {line}", loc); } -} +} // Anonymous namespace -std::string OpmInputError::formatMultiple(const std::string& reason, const std::vector& locations) { - std::vector locationStrings; - std::transform(locations.begin(), locations.end(), std::back_inserter(locationStrings), &locationStringLine); - const std::string messages { std::accumulate(locationStrings.begin(), locationStrings.end(), std::string {}) } ; +std::string +Opm::OpmInputError::formatMultiple(const std::string& reason, + const std::vector& locations) +{ + const auto messages = + std::accumulate(locations.begin(), locations.end(), std::string{}, + [](const std::string& s, const KeywordLocation& loc) + { return s + locationStringLine(loc); }); return fmt::format(R"(Problem with keywords {} -{})", messages, reason); -} - +{})", + messages, reason); } diff --git a/opm/common/utility/OpmInputError.hpp b/opm/common/utility/OpmInputError.hpp index 23182af..bf5373a 100644 --- a/opm/common/utility/OpmInputError.hpp +++ b/opm/common/utility/OpmInputError.hpp @@ -16,98 +16,95 @@ You should have received a copy of the GNU General Public License along with OPM. If not, see . */ + #ifndef OPM_ERROR_HPP #define OPM_ERROR_HPP +#include + #include #include +#include #include -#include - namespace Opm { -/* - The OpmInputError is a custom exception class which can be used to signal - errors in input handling. The importance of the OpmInputError exception is - *not* the tecnical functionality it provides, but rather the convention - surrounding it, when and how it should be used. - - The OpmInputError should be used in situations which are "close to user - input", the root cause can either be incorrect user input or a bug/limitation - in opm. OpmInputError should only be used in situations where we have a good - understanding of the underlying issue, and can provide a good error message. - - The local error handling should be complete when the OpmInputError is - instantiated, it should not be caught and rethrown in order to e.g. add - additional context or log messages. In order to avoid inadvertendly catching - this exception in a catch all block. -*/ - - - -class OpmInputError : public std::exception { +// The OpmInputError is a custom exception class which can be used to signal +// errors in input handling. The importance of the OpmInputError exception +// is *not* the tecnical functionality it provides, but rather the +// convention surrounding it, when and how it should be used. +// +// The OpmInputError should be used in situations which are "close to user +// input", the root cause can either be incorrect user input or a +// bug/limitation in opm. OpmInputError should only be used in situations +// where we have a good understanding of the underlying issue, and can +// provide a good error message. +// +// The local error handling should be complete when the OpmInputError is +// instantiated, it should not be caught and rethrown in order to e.g. add +// additional context or log messages. In order to avoid inadvertendly +// catching this exception in a catch all block. + +class OpmInputError : public std::exception +{ public: - /* - The message string will be used as format string in the fmt::format() - function as, and optional {} markers can be used to inject keyword, - filename and linenumber into the final what() message. The placeholders - must use named arguments - - {keyword} -> loc.keyword - {file} -> loc.filename - {line} -> loc.lineno - - additionally, the message can contain any number of positional - arguments to add further context to the message. - - KeywordLocation loc("KW", "file.inc", 100); - OpmInputError("Error at line {line} in file {file} - keyword: {keyword} ignored", location); - OpmInputError("Error at line {line} in file {file} - keyword: {keyword} has invalid argument {}", invalid_argument); - */ - - template - OpmInputError(const std::string& reason, const KeywordLocation& location, const Args& ...furtherLocations) - : locations { location, furtherLocations... } + // The message string will be used as format string in the fmt::format() + // function as, and optional {} markers can be used to inject keyword, + // filename and line numbers into the final what() message. The + // placeholders may use one or more of the following named arguments + // + // {keyword} -> location.keyword + // {file} -> location.filename + // {line} -> location.lineno + // + // Furthermore, the message can contain any number of positional + // arguments to add further context to the message. + // + // KeywordLocation loc("KW", "file.inc", 100); + // OpmInputError("Error at line {line} in file {file} - keyword: {keyword} ignored", location); + // OpmInputError("Error at line {line} in file {file} - keyword: {keyword} has invalid argument {}", invalid_argument); + + template + OpmInputError(const std::string& reason, + const KeywordLocation& location, + Args&&... furtherLocations) + : locations { location, std::forward(furtherLocations)... } , m_what { - locations.size() == 1 - ? formatSingle(reason, locations[0]) + (locations.size() == 1) + ? formatSingle(reason, locations.front()) : formatMultiple(reason, locations) } - { } - - /* - Allows for the initialisation of an OpmInputError from another exception. - - Usage: - - try { - . - . - . - } catch (const Opm::OpmInputError&) { - throw; - } catch (const std::exception& e) { - std::throw_with_nested(Opm::OpmInputError(e, location)); - } - */ + {} + + // Allows for the initialisation of an OpmInputError from another exception. + // + // Usage: + // + // try { + // . + // . + // . + // } catch (const Opm::OpmInputError&) { + // throw; + // } catch (const std::exception& e) { + // std::throw_with_nested(Opm::OpmInputError(e, location)); + // } OpmInputError(const std::exception& error, const KeywordLocation& location) : locations { location } - , m_what { formatException(error, locations[0]) } - { } + , m_what { formatException(error, locations.front()) } + {} - const char * what() const throw() + const char* what() const noexcept override { return this->m_what.c_str(); } - static std::string format(const std::string& msg_format, const KeywordLocation& loc); private: // The location member is here for debugging; depending on the msg_fmt - // passed in the constructor we might not have captured all the information - // in the location argument passed to the constructor. + // passed in the constructor we might not have captured all the + // information in the location argument passed to the constructor. std::vector locations; std::string m_what; @@ -117,5 +114,6 @@ private: static std::string formatMultiple(const std::string& reason, const std::vector&); }; -} -#endif +} // namespace Opm + +#endif // OPM_ERROR_HPP diff --git a/opm/input/eclipse/Parser/Parser.cpp b/opm/input/eclipse/Parser/Parser.cpp index bda949f..1d53d83 100644 --- a/opm/input/eclipse/Parser/Parser.cpp +++ b/opm/input/eclipse/Parser/Parser.cpp @@ -659,27 +659,31 @@ void ParserState::loadFile(const std::filesystem::path& inputFile) { * of the data section of any keyword. */ -void ParserState::handleRandomText(const std::string_view& keywordString ) const { - std::string errorKey; - std::string trimmedCopy = std::string( keywordString ); - std::string msg; - KeywordLocation location{lastKeyWord, this->current_path(), this->line()}; +void ParserState::handleRandomText(const std::string_view& keywordString) const +{ + const std::string trimmedCopy { keywordString }; + const KeywordLocation location { + lastKeyWord, this->current_path(), this->line() + }; + std::string errorKey{}; + std::string msg{}; if (trimmedCopy == "/") { errorKey = ParseContext::PARSE_RANDOM_SLASH; msg = "Extra '/' detected in {file} line {line}"; } else if (lastSizeType == OTHER_KEYWORD_IN_DECK) { - errorKey = ParseContext::PARSE_EXTRA_RECORDS; - msg = "Too many records in keyword {keyword}\n" - "In {} line {}"; + errorKey = ParseContext::PARSE_EXTRA_RECORDS; + msg = "Too many records in keyword {keyword}\n" + "In {file} line {line}"; } else { errorKey = ParseContext::PARSE_RANDOM_TEXT; msg = fmt::format("String {} not formatted as valid keyword\n" "In {{file}} line {{line}}.", keywordString); } - parseContext.handleError( errorKey , msg, location, errors ); + + parseContext.handleError(errorKey, msg, location, errors); } diff --git a/tests/parser/integration/ParseKEYWORD.cpp b/tests/parser/integration/ParseKEYWORD.cpp index f31b3b0..516e3a3 100644 --- a/tests/parser/integration/ParseKEYWORD.cpp +++ b/tests/parser/integration/ParseKEYWORD.cpp @@ -50,6 +50,12 @@ #include #include +#include +#include +#include +#include +#include + using namespace Opm; namespace { @@ -236,14 +242,13 @@ SGCWMIS BOOST_AUTO_TEST_CASE( SORWMIS ) { - Parser parser; - // missing miscible keyword - BOOST_CHECK_THROW (parser.parseString(sorwmisData), OpmInputError ); + // Missing miscible keyword + BOOST_CHECK_THROW(Parser{}.parseString(sorwmisData), OpmInputError); - //too many tables - BOOST_CHECK_THROW( parser.parseString(miscibleTightData + sorwmisData), OpmInputError); + // Too many tables + BOOST_CHECK_THROW(Parser{}.parseString(miscibleTightData + sorwmisData), OpmInputError); - auto deck1 = parser.parseString(miscibleData + sorwmisData); + const auto deck1 = Parser{}.parseString(miscibleData + sorwmisData); const auto& sorwmis = deck1["SORWMIS"].back(); const auto& miscible = deck1["MISCIBLE"].back(); @@ -253,9 +258,9 @@ BOOST_AUTO_TEST_CASE( SORWMIS ) { const auto& sorwmis1 = sorwmis.getRecord(1); // test number of columns - size_t ntmisc = miscible0.getItem(0).get< int >(0); - Opm::SorwmisTable sorwmisTable0(sorwmis0.getItem(0), 0); - BOOST_CHECK_EQUAL(sorwmisTable0.numColumns(),ntmisc); + const std::size_t ntmisc = miscible0.getItem(0).get(0); + const Opm::SorwmisTable sorwmisTable0(sorwmis0.getItem(0), 0); + BOOST_CHECK_EQUAL(sorwmisTable0.numColumns(), ntmisc); // test table input 1 BOOST_CHECK_EQUAL(3U, sorwmisTable0.getWaterSaturationColumn().size()); @@ -263,7 +268,7 @@ BOOST_AUTO_TEST_CASE( SORWMIS ) { BOOST_CHECK_EQUAL(0.0, sorwmisTable0.getMiscibleResidualOilColumn()[2]); // test table input 2 - Opm::SorwmisTable sorwmisTable1(sorwmis1.getItem(0), 1); + const Opm::SorwmisTable sorwmisTable1(sorwmis1.getItem(0), 1); BOOST_CHECK_EQUAL(sorwmisTable1.numColumns(),ntmisc); BOOST_CHECK_EQUAL(3U, sorwmisTable1.getWaterSaturationColumn().size());