fixed missing exception handling in MathLib::toBig{U}Number()#8025
fixed missing exception handling in MathLib::toBig{U}Number()#8025firewave merged 1 commit intodanmar:mainfrom
MathLib::toBig{U}Number()#8025Conversation
|
|
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.
can we discuss some builtin exceptions. If there is anything like In python a good reason you don't want to catch I don't see that stuff will stop working in cppcheck because we catch std::exception. |
That's why I started documenting all of them in Cppcheck (PR coming soon). With the help of more tests and clang-tidy we can certainly improve some cases.
It is just cleaner. |
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
I am not very excited.. but well chatgpt also told me that it's best practice to do it like this.
|
After using a fuzzer to trigger certain errors in another PR having a more narrow handling would allow you for undefined behavior but also unexpected errors. I also plan to do for for the important parts of the code. And after introducing That was actually the function in question and I did not produce another exception but the one we are throwing. I am also confident it might not happen since I had to fuzz a long time for some of our own exceptions to happen. The MathLib functions are also great functions to fuzz.
That would me actually make this reconsider as AI is usually wrong... |
2 participants
No description provided.