Display more helpful error message when parsing a BLIF file that cont… #340
Conversation
…ains multi-driven nets. Fixes verilog-to-routing#330.
probot-autolabeler
bot
added
lang-cpp
There was a problem hiding this comment.
Thanks for the patch!
The added check looks like it handles the case where a .names drives an already-driven net.
However to truly fix #330, it should also check anywhere else a net's driver is assigned. That would include: .inputs, .latchs and .subckts.
The same technique you used should be applicable. To avoid code duplication I'd suggest wrapping up the check in a re-useable function which can just be called from the input/latch/subckt/names creation functions.
probot-autolabeler
bot
added
lang-cpp
|
Made those fixes. Thanks for taking the time to double-check me! |
|
Are you adding regression tests to check error messages? If so, could you point me at an existing test and I'll base a test off it. |
| void assert_net_has_no_driver(std::string net_name) { | ||
| AtomNetId net_id = curr_model().create_net(net_name); | ||
| assert_net_has_no_driver(net_id, net_name); | ||
| } |
There was a problem hiding this comment.
We can probably drop this overload if we restructure the subckt check slightly (see below).
| subckt_name = nets[i]; | ||
| subckt_name_set = true; | ||
| } | ||
| assert_net_has_no_driver(nets[i]); |
There was a problem hiding this comment.
Likely best to move this check down to after we have the actual net_id from create_net(). We can still figure out if it's an output by looking at pin_type.
|
It would be great to add a regression test for this case. A short tutorial on adding tests is here: https://docs.verilogtorouting.org/en/latest/dev/developing/#adding-tests Generally our tests have focused on verifying functionality on correct input. It would be good to start building up tests which checks error handling. I think the test scripts should handle checking that the tool exits with an appropriate non-zero exit code (VPR should exit with 1 on an error like this). But if they don't we can merge this PR and address that separately. |
|
Hi @benreynwar, Any idea if this change is still relevant? It would be nice to rebase and merge it if it is? Tim 'mithro' Ansell |
…ains multi-driven nets. Fixes #330.
Description
When parsing a blif file into a netlist, a check is added to make sure that the blif file does not contain any multi-driven nets.
Related Issue
Fixes issue #330.
Motivation and Context
Gives user a more helpful error message.
How Has This Been Tested?
Confirmed that the example blif in issue #330 produces the new error message.
I have not added a regression test.
Types of changes
This change should just modify the contents of the error message that the user receives.
Checklist:
I ran the basic regression tests without failure. Strong are currently running.