Remove C++ exceptions from SystemTimeFormatter and derived classes#45372
Remove C++ exceptions from SystemTimeFormatter and derived classes#45372yanavlasov wants to merge 2 commits into
Conversation
Signed-off-by: Yan Avlasov <yavlasov@google.com>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors SystemTimeFormatter and its derived classes to use factory methods returning absl::StatusOr instead of throwing exceptions during construction. The reviewer feedback suggests removing the redundant checkConstructPreconditions static methods from all derived classes, as they can inherit this behavior directly from SystemTimeFormatter and be accessed via the friended makeTimeFormatter factory function.
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
LGTM overall except one comment. Thanks, Yan, for working on this.
| absl::StatusOr<std::unique_ptr<T>> makeTimeFormatter(absl::string_view format) { | ||
| static_assert(std::is_base_of<SystemTimeFormatter, T>::value, | ||
| "T must be derived from SystemTimeFormatter"); | ||
| RETURN_IF_NOT_OK(T::checkConstructPreconditions(format)); |
There was a problem hiding this comment.
why we need to add new checkConstructPreconditions() to every derived class because seems they are completely same?
There was a problem hiding this comment.
Ok, I have removed them.
Signed-off-by: Yan Avlasov <yavlasov@google.com>
2 participants
Replace constructors that can throw with a factory method that returns StatusOr<> for SystemTimeFormatter and derived classes. There should be no functional changes, as there is still a throw present when a factory method fails.
Test changes are all conversions from direct construction to a factory method.
In the subsequent PRs I will generalize the makeTimeFormatter so it can be used with generic types.
There are boilerplate checkConstructPreconditions method in the derived classes that just call the base class. They are not strictly needed, and I have left them, just to so I can use this as pattern for agents that will work on implementing this pattern for other types where derived classes have their own preconditions.
Risk Level: low
Testing: unit tests
Docs Changes: no
Release Notes: no
Platform Specific Features: no