fixed missing exception handling in MathLib::toBig{U}Number()

[firewave]

No description provided.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
85.7% Duplication on New Code

See analysis details on SonarQube Cloud

[firewave]

Uncovered by adding @throws documentation to simplecpp: danmar/simplecpp#600.

[danmar]

why? if there is some c++ exception it sounds good to handle that?

[firewave]

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

[danmar]

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.

[danmar]

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.

[firewave]

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.

[danmar]

That's how we treat exceptions.

can we discuss some builtin exceptions. If there is anything like std::out_of_range then I would prefer an internal error that points at the token. I don't see why we wouldn't want to get context if there is out_of_range exceptions.

In python a good reason you don't want to catch Exception is that it can mean anything including Ctrl+C and by catching Exception it's possible that Ctrl+C stops working.

I don't see that stuff will stop working in cppcheck because we catch std::exception.

[firewave]

can we discuss some builtin exceptions. If there is anything like std::out_of_range then I would prefer an internal error that points at the token. I don't see why we wouldn't want to get context if there is out_of_range exceptions.

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.

I don't see that stuff will stop working in cppcheck because we catch std::exception.

It is just cleaner.

[danmar]

I am not very excited.. but well chatgpt also told me that it's best practice to do it like this.

[firewave]

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 noexcept in more places that would be easier as it would crash out.

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.

I am not very excited.. but well chatgpt also told me that it's best practice to do it like this.

That would me actually make this reconsider as AI is usually wrong...