fixed missing exception handling in MathLib::toBig{U}Number()
#8025
Conversation
|
|
Uncovered by adding |
| try { | ||
| return simplecpp::characterLiteralToLL(str); | ||
| } catch (const std::exception& e) { | ||
| } catch (const std::runtime_error& e) { |
There was a problem hiding this comment.
why? if there is some c++ exception it sounds good to handle that?
There was a problem hiding this comment.
Using std::exception might imply it might throw more than a specific type which is not the case.
It is also better to have narrow exceptions (IMO - also a best practice in Java/Python - which have better annotations to be fair).
There was a problem hiding this comment.
Using std::exception might imply it might throw more than a specific type which is not the case.
I fear you only looked at what exceptions the functions are throwing explicitly.
It is also better to have narrow exceptions (IMO - also a best practice in Java/Python - which have better annotations to be fair).
Imho it's a bad idea in Python to catch generic Exception because that will catch everything including when Ctrl+C is pressed. Yes it can be too broad.
There was a problem hiding this comment.
If your intention is: "if there is a C++ exception here then you want it to be catched in the CppCheck instead of throwing an InternalError" then this would be the proper change. But I wouldn't see the point of that.
There was a problem hiding this comment.
I fear you only looked at what exceptions the functions are throwing explicitly.
That's how we treat exceptions.
If your intention is: "if there is a C++ exception here then you want it to be catched in the CppCheck instead of throwing an InternalError" then this would be the proper change. But I wouldn't see the point of that.
It was inconsistent and the InternalError also takes the token so if we were encountering this exception the location which caused it would have been missing.
I will noexcept the simplecpp interface in a follow-up (still waiting for llvm/llvm-project#168324 to be merged).
This is also the only function which currently throws and for consistency we should probably adjust that - but that is something for the future.
2 participants
No description provided.