util: Make strprintf noexcept. Improve error message on format string error.

practicalswift commented at 3:13 pm on June 4, 2018: contributor

Make strprintf noexcept. Improve the error message given on format string errors.

With this change large parts of the code base (strprintf callers directly or indirectly) are switched from an implicit “might throw tinyformat::format_error” to “will not throw” from a compiler/static analyzer perspective.

Also, reducing the number of unnecessary exceptions thrown increases the signal-to-noise for humans when analyzing potential issues introduced by uncaught exceptions.

These were the conditions that could make strprintf(…) throw tinyformat::format_error prior to this commit:

 0$ git grep TINYFORMAT_ERROR
 1src/tinyformat.h:// Error handling: Define TINYFORMAT_ERROR to customize the error handling for
 2src/tinyformat.h:#define TINYFORMAT_ERROR(reasonString) throw tinyformat::format_error(reasonString)
 3src/tinyformat.h:#ifndef TINYFORMAT_ERROR
 4src/tinyformat.h:#   define TINYFORMAT_ERROR(reason) assert(0 && reason)
 5src/tinyformat.h:        TINYFORMAT_ERROR("tinyformat: Cannot convert from argument type to "
 6src/tinyformat.h:        TINYFORMAT_ERROR("tinyformat: Not enough conversion specifiers in format string");
 7src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: Not enough arguments to read variable width");
 8src/tinyformat.h:                TINYFORMAT_ERROR("tinyformat: Not enough arguments to read variable precision");
 9src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: the %a and %A conversion specs "
10src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: %n conversion spec not supported");
11src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: Conversion spec incorrectly "
12src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: Not enough format arguments");
13src/tinyformat.h:        TINYFORMAT_ERROR("tinyformat: Too many conversion specifiers in format string");

practicalswift force-pushed on Jun 4, 2018

util: Make strprintf noexcept. Improve error message on format string error. 98b0198d2a

practicalswift force-pushed on Jun 4, 2018

Empact commented at 8:46 am on June 5, 2018: member

Mild NACK I prefer noisy failures. If the failures here are burdensome, can you cite some?

practicalswift commented at 9:36 am on June 5, 2018: contributor

@Empact How is this less noisy than before? :-)

0-    format(oss, fmt.c_str(), args...);
1+    try {
2+        tfm::format(oss, fmt.c_str(), args...);
3+    } catch (const tinyformat::format_error& fmterr) {
4+        fprintf(stderr, "Format error: %s (fmt=\"%s\")\n", fmterr.what(), fmt.c_str());
5+        std::terminate();
6+    }

Empact commented at 9:39 am on June 5, 2018: member

Basically, blow up when possible, return a value or log or both when not possible. Make errors as difficult to ignore as possible.

practicalswift commented at 9:40 am on June 5, 2018: contributor

@Empact I agree, but I don’t get how this is less noisy. std::terminate() is called earlier :-)

Empact commented at 10:03 am on June 5, 2018: member

Fair enough, terminate is effectively noisy, but given terminate would be called if the exception were uncaught, ironically, this now makes errors which would have previously been blanket-caught via catch(...) etc a terminate which is a potentially problematic behavior change. But maybe I’m wrong on that? It was called out on me here relative to converting asserts to throw: #13268

FTR I don’t like that we have blanket-catches in the codebase, but they’re sometimes warranted.

fanquake added the label Utils/log/libs on Jul 1, 2018

practicalswift commented at 11:58 am on July 20, 2018: contributor

Hopefully we will have format string linting soon (see separate PR) and perhaps that will allow us to make strprintf(…) noexcept. But I’m not sure std::terminate is the right way to go here in case of formatting errors. Perhaps better to just print an error message to stderr and/or return the error message to the caller? @laanwj, what do you think?

DrahtBot commented at 3:16 pm on July 29, 2018: member

DrahtBot closed this on Jul 29, 2018

DrahtBot reopened this on Jul 29, 2018

DrahtBot commented at 11:43 pm on August 1, 2018: member

#13846 (Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact)
#13845 (Include tinyformat as a subtree by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 11:03 am on August 31, 2018: member

I’m going to close this, I don’t think this change is really necessary.

laanwj closed this on Aug 31, 2018

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 18, 2022

util: Make strprintf noexcept. Improve error message on format string error. #13392