Skip to content
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

Jump to bottom

Replace empty check of error, warning and info list with check for errors only #15203

Merged
merged 1 commit into from
Sep 9, 2024
Merged

Replace empty check of error, warning and info list with check for errors only #15203

merged 1 commit into from
Sep 9, 2024

Conversation

matheusaaguiar
Copy link
Collaborator

This is a minor bug found here by @cameel.
There are some instances where an intended check for errors can incorrectly consider warnings and info as well.

@matheusaaguiar matheusaaguiar self-assigned this Jun 18, 2024
matheusaaguiar
matheusaaguiar commented Jun 18, 2024
View reviewed changes
Copy link
Collaborator Author

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

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

There is also an instance of using errors().empty() check at

solidity/libsolidity/formal/ModelChecker.cpp

Line 142 in d0190e1

else if (!m_unsupportedErrorReporter.errors().empty())

but in this case it is specifically used to collect only warnings in SMTEncoder, BMC and CHC.

libsolidity/interface/StandardCompiler.cpp Show resolved Hide resolved
@matheusaaguiar matheusaaguiar marked this pull request as ready for review June 18, 2024 04:42
@matheusaaguiar matheusaaguiar mentioned this pull request Jun 18, 2024
Merged
cameel
cameel reviewed Jun 20, 2024
View reviewed changes
libsolidity/codegen/CompilerContext.cpp Show resolved Hide resolved
libsolidity/interface/StandardCompiler.cpp Show resolved Hide resolved
libsolidity/interface/StandardCompiler.cpp Show resolved Hide resolved
test/libsolidity/InlineAssembly.cpp Outdated Show resolved Hide resolved
test/libsolidity/InlineAssembly.cpp Outdated Show resolved Hide resolved
test/libyul/Common.cpp Outdated Show resolved Hide resolved
test/tools/yulrun.cpp Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 8, 2024

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jul 8, 2024
@matheusaaguiar matheusaaguiar removed the stale The issue/PR was marked as stale because it has been open for too long. label Jul 8, 2024
@github-actions GitHub Actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jul 23, 2024
@matheusaaguiar matheusaaguiar removed the stale The issue/PR was marked as stale because it has been open for too long. label Jul 23, 2024
@matheusaaguiar matheusaaguiar force-pushed the replaceErrorsEmptyWithHasErrors branch from 19b414c to d290eb2 Compare August 1, 2024 19:05
cameel
cameel reviewed Aug 1, 2024
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

One of the changes is still incorrect. See this thread: #15203 (comment)

liblangutil/Exceptions.h Show resolved Hide resolved
libsolidity/interface/StandardCompiler.cpp Show resolved Hide resolved
@matheusaaguiar matheusaaguiar force-pushed the replaceErrorsEmptyWithHasErrors branch 2 times, most recently from 2e56620 to 5dbe3b7 Compare August 2, 2024 16:46
@ekpyron ekpyron added the 🟡 PR review label label Aug 15, 2024
@github-actions GitHub Actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Aug 30, 2024
@matheusaaguiar matheusaaguiar removed the stale The issue/PR was marked as stale because it has been open for too long. label Aug 30, 2024
cameel
cameel previously approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

This now looks fine. Just needs a rebase and we can merge.

Since this was sitting for quite a while, before merging I'd recommend a quick check whether a new instance of .errors().empty() hasn't appeared somewhere.

liblangutil/ErrorReporter.h Outdated Show resolved Hide resolved
PHAPKAN168 referenced this pull request Sep 5, 2024
@ekpyron
Update bugs by version entry for 0.8.20.
a1b79de
@cameel cameel force-pushed the replaceErrorsEmptyWithHasErrors branch from efa3728 to 13fe5ba Compare September 9, 2024 12:31
@cameel cameel merged commit ac81d42 into develop Sep 9, 2024
70 of 71 checks passed
@cameel cameel deleted the replaceErrorsEmptyWithHasErrors branch September 9, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel approved these changes

Assignees

@matheusaaguiar matheusaaguiar

Labels
bug 🐛 🟡 PR review label
Projects
Solidity Focus Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matheusaaguiar @cameel @ekpyron