Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
firewave merged 1 commit into from
Dec 15, 2025
+22 −6
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions lib/mathlib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,13 @@ MathLib::biguint MathLib::toBigUNumber(const std::string & str, const Token * co
return static_cast<biguint>(static_cast<bigint>(doubleval));
}

if (isCharLiteral(str))
return simplecpp::characterLiteralToLL(str);
if (isCharLiteral(str)) {
try {
return simplecpp::characterLiteralToLL(str);
} catch (const std::runtime_error& e) {
throw InternalError(tok, "Internal Error. MathLib::toBigUNumber: characterLiteralToLL(" + str + ") => " + e.what());
}
}

try {
std::size_t idx = 0;
Expand Down Expand Up @@ -429,8 +434,13 @@ MathLib::bigint MathLib::toBigNumber(const std::string & str, const Token * cons
return static_cast<bigint>(doubleval);
}

if (isCharLiteral(str))
return simplecpp::characterLiteralToLL(str);
if (isCharLiteral(str)) {
try {
return simplecpp::characterLiteralToLL(str);
} catch (const std::runtime_error& e) {
throw InternalError(tok, "Internal Error. MathLib::toBigNumber: characterLiteralToLL(" + str + ") => " + e.what());
}
}

try {
std::size_t idx = 0;
Expand Down Expand Up @@ -505,7 +515,7 @@ double MathLib::toDoubleNumber(const std::string &str, const Token * const tok)
if (isCharLiteral(str)) {
try {
return simplecpp::characterLiteralToLL(str);
} catch (const std::exception& e) {
} catch (const std::runtime_error& e) {
Copy link
Owner

@danmar danmar Dec 10, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@firewave firewave Dec 10, 2025

Choose a reason for hiding this comment

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

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

Copy link
Owner

@danmar danmar Dec 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@danmar danmar Dec 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@firewave firewave Dec 12, 2025

Choose a reason for hiding this comment

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

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.

throw InternalError(tok, "Internal Error. MathLib::toDoubleNumber: characterLiteralToLL(" + str + ") => " + e.what());
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/tokenize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3470,7 +3470,7 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration, int fileIndex)
if (tok->tokType() == Token::eChar && tok->values().empty()) {
try {
simplecpp::characterLiteralToLL(tok->str());
} catch (const std::exception &e) {
} catch (const std::runtime_error &e) {
unhandledCharLiteral(tok, e.what());
}
}
Expand Down
6 changes: 6 additions & 0 deletions test/testmathlib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,8 @@ class TestMathLib : public TestFixture {
TokenList::deleteTokens(tok);
}

ASSERT_THROW_INTERNAL_EQUALS(MathLib::toBigNumber("''"), INTERNAL, "Internal Error. MathLib::toBigNumber: characterLiteralToLL('') => empty character literal");

// TODO: test binary
// TODO: test floating point

Expand Down Expand Up @@ -588,6 +590,8 @@ class TestMathLib : public TestFixture {
TokenList::deleteTokens(tok);
}

ASSERT_THROW_INTERNAL_EQUALS(MathLib::toBigUNumber("''"), INTERNAL, "Internal Error. MathLib::toBigUNumber: characterLiteralToLL('') => empty character literal");

// TODO: test binary
// TODO: test floating point

Expand Down Expand Up @@ -736,6 +740,8 @@ class TestMathLib : public TestFixture {
ASSERT_EQUALS("0.0", MathLib::toString(MathLib::toDoubleNumber("-0")));
ASSERT_EQUALS("0.0", MathLib::toString(MathLib::toDoubleNumber("-0.")));
ASSERT_EQUALS("0.0", MathLib::toString(MathLib::toDoubleNumber("-0.0")));

ASSERT_THROW_INTERNAL_EQUALS(MathLib::toDoubleNumber("''"), INTERNAL, "Internal Error. MathLib::toDoubleNumber: characterLiteralToLL('') => empty character literal");
}

void isint() const {
Expand Down
Loading