Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 30, 2019

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.

@laanwj
Copy link
Member

laanwj commented Apr 30, 2019

Are you intending to use this in other places than logging?
If not, I'd prefer to keep bitcoin core specific code outside tinyformat.h.
ACK on reverting the style changes.

@maflcko
Copy link
Member Author

maflcko commented Apr 30, 2019

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!")

@maflcko maflcko force-pushed the 1904-tinyformatNoThrow branch from fac6cbe to faf7b04 Compare April 30, 2019 18:27
}

template <typename... Args>
std::string format_no_throw(const std::string& fmt, const Args&... args)
Copy link
Contributor

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 :-)

Copy link
Member Author

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?

Copy link
Contributor

@practicalswift practicalswift May 2, 2019

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?

Copy link
Member

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.

Copy link
Contributor

@practicalswift practicalswift May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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

Copy link
Contributor

@practicalswift practicalswift May 3, 2019

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.

Copy link
Contributor

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.

Copy link
Member Author

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...));
Copy link
Contributor

@practicalswift practicalswift May 1, 2019

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):

  • LogPrintStrLogTimestampStrFormatISO8601DateTimestrprintf

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 in FormatISO8601DateTime and FormatISO8601Date
  • Add noexcept to FormatISO8601DateTime, FormatISO8601Date and LogPrintf

@maflcko maflcko force-pushed the 1904-tinyformatNoThrow branch from a157831 to faf7b04 Compare May 1, 2019 13:49
@DrahtBot
Copy link
Contributor

DrahtBot commented May 9, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@maflcko maflcko force-pushed the 1904-tinyformatNoThrow branch from faf7b04 to fa6d1bd Compare May 14, 2019 13:09
@maflcko maflcko force-pushed the 1904-tinyformatNoThrow branch from fa6d1bd to fa7e309 Compare June 10, 2019 07:03
@maflcko maflcko closed this Jun 14, 2019
@maflcko maflcko deleted the 1904-tinyformatNoThrow branch June 14, 2019 09:55
@maflcko
Copy link
Member Author

maflcko commented Jun 14, 2019

We have a linter to check the number of arguments, so I guess this isn't much needed

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants