-
Notifications
You must be signed in to change notification settings - Fork 38.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
src/tinyformat.h
Outdated
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
src/logging.h
Outdated
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_errorinFormatISO8601DateTimeandFormatISO8601Date - Add
noexcepttoFormatISO8601DateTime,FormatISO8601DateandLogPrintf
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_throwthat (on error) returns the format string with the error message.format_no_throwis 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.