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.

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

Reduce number of warnings #4784

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Reduce number of warnings #4784

wants to merge 2 commits into from

Conversation

KrystalDelusion
Copy link
Member

@KrystalDelusion KrystalDelusion commented Nov 28, 2024

What are the reasons/motivation for this change?
There are a number of easy to fix warnings:

  1. comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Wsign-compare]

Explain how this is achieved.

  1. Const::size() returns int, so change iterators that use it to auto instead of size_t. For cases where size is being explicitly cast to int, use the wrapper that we already have instead: Yosys::GetSize().

If applicable, please suggest to reviewers how they can test the change.
Check what/if any warnings still show up at the bottom of the "Files changed" tab and in the "Compiler testing" section of the checks

`Const::size()` returns int, so change iterators that use it to `auto` instead of `size_t`.
For cases where size is being explicitly cast to `int`, use the wrapper that we already have instead: `Yosys::GetSize()`.
@KrystalDelusion
Copy link
Member Author

@povik Do you want me to tidy up the warnings in the aiger2 code?

@povik
Copy link
Member

povik commented Dec 1, 2024

Yes please

- Remove unused statics CONST_FALSE and CONST_TRUE (which appear to have been folded into the `Index` declaration as CFALSE and CTRUE).
- Assign default value of EMPTY_LIT to `a` and `b` for comparison ops.
- Tag debug only variables with YS_MAYBE_UNUSED, don't assign unused variables (but continue to call the function because it moves the file pointer).
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

OK on the aiger2 changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants