-
Notifications
You must be signed in to change notification settings - Fork 37.7k
tinyformat: Add format_no_throw #15926
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
Conversation
Are you intending to use this in other places than logging? |
Yes, the gui should probably use it for messages, so that at least the generic error is logged (as opposed to only "A tfm error occured!") |
fac6cbe
to
faf7b04
Compare
} | ||
|
||
template <typename... Args> | ||
std::string format_no_throw(const std::string& fmt, const Args&... args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add noexcept
to make the "no throw" part of the contract and thus understandable also for the compiler :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I (or the compiler) be sure that no other exceptions (e.g. std::out_of_range
) are thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is really no shortcut here AFAIK: reading + reasoning would be required :-)
Do you see any places where e.g. std::out_of_range
could be thrown that is reachable from format
? Beyond the already handled format_error
.
The contract of tinyformat
is that format_error
is the only exception thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point of adding "noexcept" isn't so much because you know a function won't throw; it's that you know that exceptions thrown anyway can be regarded as fatal errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stroustrup and Sutter:
E.12: Use noexcept when exiting a function because of a throw is impossible or unacceptable
format_no_throw
seems like an ideal candidate for noexcept
:-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so it is equivalent to std::terminate
. I think I will leave that for later, as I am not sure that immediate termination is better than offering the gui or daemon to catch the exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is unwise to use function naming (format_no_throw
) which gives the caller the impression that the contract is that the function cannot throw if we're not willing to back that with a noexcept
guarantee.
I think we should either 1.) handle any non-fatal exception within format_no_throw
and mark it noexcept
, or 2.) remove the _no_throw
suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment like This function wraps format() to return error messages instead of throwing format_error. All other format() exceptions are thrown away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment
log_msg = "Error \"" + std::string(fmterr.what()) + "\" while formatting log message: " + fmt; | ||
} | ||
LogInstance().LogPrintStr(log_msg); | ||
LogInstance().LogPrintStr(tfm::format_no_throw(fmt, args...)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making LogPrintf
noexcept
, but before doing so note that the following call chain is triggered here (also before this PR):
LogPrintStr
→LogTimestampStr
→FormatISO8601DateTime
→strprintf
Generally strprintf
may throw tinyformat::format_error
.
In the case of the call FormatISO8601DateTime
I don't think that is possible since the format string is well formed. However, that is not understood by the compiler which currently must assume that FormatISO8601DateTime
can throw.
I suggest handling the (non-reachable) tinyformat::format_error
explicitly in FormatISO8601DateTime
and then making that function noexcept
too. (Should be done also for its companion FormatISO8601Date
).
So in summary my suggestion is to:
- Handle
tinyformat::format_error
inFormatISO8601DateTime
andFormatISO8601Date
- Add
noexcept
toFormatISO8601DateTime
,FormatISO8601Date
andLogPrintf
a157831
to
faf7b04
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
faf7b04
to
fa6d1bd
Compare
fa6d1bd
to
fa7e309
Compare
We have a linter to check the number of arguments, so I guess this isn't much needed |
This adds a
tinyformat::format_no_throw
that (on error) returns the format string with the error message.format_no_throw
is currently only used in logging, but can be used by other modules after this patch.The fist commit reverts some style-changes that have been made to tinyformat, which is to be treated like a "subtree", so style-fixes should go upstream and are not acceptable in Bitcoin Core.