Skip to content

Conversation

@lee30sonia
Copy link
Member

Only add the clauses for a node when it is used.
Invocation of push/pop is adjusted (only use it for duplicated TFO cone and when validating with non-existing circuit).
Documentation is also updated, referring to cnf_view.

I would suggest to wait until this PR is merged and also port it here. https://github.com/lsils/bill/pull/35

@lee30sonia lee30sonia requested review from hriener and msoeken and removed request for msoeken June 11, 2020 17:05
@coveralls
Copy link

coveralls commented Jun 11, 2020

Pull Request Test Coverage Report for Build 133198198

  • 58 of 70 (82.86%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.05%) to 79.931%

Changes Missing Coverage Covered Lines Changed/Added Lines %
include/mockturtle/algorithms/circuit_validator.hpp 56 68 82.35%
Files with Coverage Reduction New Missed Lines %
include/mockturtle/algorithms/cnf.hpp 3 59.3%
Totals Coverage Status
Change from base Build 132216424: 0.05%
Covered Lines: 8742
Relevant Lines: 10937

💛 - Coveralls

@hriener
Copy link
Member

hriener commented Jun 12, 2020

This looks also very good to me.

Some more minor comments:

  • I would prefer more descriptive names for variables (e.g., index instead of idx or inverted instead of inv, circuit instead of ckt, etc.).
  • Is it possible to use initializer_lists for fanin? E.g. instead of
    circuit_validator<aig_network>::gate::fanin gi1; gi1.idx = 0; gi1.inv = true;
    you may be able to write
    circuit_validator<aig_network>::gate::fanin gi1{0,true};

@lee30sonia
Copy link
Member Author

  • Is it possible to use initializer_lists for fanin? E.g. instead of
    circuit_validator<aig_network>::gate::fanin gi1; gi1.idx = 0; gi1.inv = true;
    you may be able to write
    circuit_validator<aig_network>::gate::fanin gi1{0,true};

I remember this syntax is not available with some operating system / compiler ?
After all, this only appears in the example in the documentation and the test, so I think it doesn't affect too much.

@hriener
Copy link
Member

hriener commented Jun 12, 2020

  • Is it possible to use initializer_lists for fanin? E.g. instead of
    circuit_validator<aig_network>::gate::fanin gi1; gi1.idx = 0; gi1.inv = true;
    you may be able to write
    circuit_validator<aig_network>::gate::fanin gi1{0,true};

I remember this syntax is not available with some operating system / compiler ?
After all, this only appears in the example in the documentation and the test, so I think it doesn't affect too much.

I think you confuse aggregate initialization [1] (C++20) with list initialization [2] (C++11). If list initialization works, then I would prefer it!

[1] https://en.cppreference.com/w/cpp/language/aggregate_initialization
[2] https://en.cppreference.com/w/cpp/language/list_initialization

@hriener
Copy link
Member

hriener commented Jun 12, 2020

Is the PR ready to be merged?

@lee30sonia
Copy link
Member Author

Yes, I think so.

@hriener hriener merged commit 0ec1b6d into lsils:master Jun 12, 2020
@hriener hriener deleted the incremental_CNF branch June 12, 2020 10:38
@hriener
Copy link
Member

hriener commented Jun 12, 2020

Thanks!

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