Open cfib opened 5 years ago
This looks to be a bug, thank you for pointing this out. I agree, the cell type should most certainly be included in checking for equality among two instances of EDIFCellInst
. I just surveyed the EDIF objects and how they implement equality as this may have not been fully examined previously:
EDIF Object | Equality check |
---|---|
EDIFCell |
EDIFName.equals() |
EDIFCellInst |
EDIFName.equals() |
EDIFDesign |
EDIFName.equals() |
EDIFHierCellInst |
EDIFName.equals() |
EDIFHierNet |
EDIFNet.equals() && hierarchicalInstName.equals() |
EDIFHierPortInst |
EDIFPortInst.equals() && hierarchicalInstName.equals() |
EDIFLibrary |
EDIFName.equals() |
EDIFNet |
EDIFName.equals() |
EDIFPort |
name.equals() && EDIFCellInst.equals() |
EDIFPortInst |
EDIFName.equals() |
EDIFPropertyObject |
EDIFName.equals() |
EDIFPropertyValue |
Object.equals() |
I recommend the following changes take place:
EDIFCellInst.equals()
be implemented to include equality of its cell type as well as the name.EDIFHierCellInst.equals()
should be implemented to match behavior of EDIFHierNet
and EDIFHierPortInst
.EDIFPropertyValue
should have equality checked by both the type
and value
rather than defaulting to the Object.equals()
behavior.EDIFPropertyObject
should also check the property sets match as well. This would impact most all other objects that can be decorated with properties.One other idea that might be relevant is that based on circumstances, one might want a deepEquals()
method that recursively checks all children objects of EDIFCell
objects. These would be more expensive checks but could save a lot of code if it was used for doing netlist 'diff' type operations.
Could you share any insights into why you would like to check if cell instances are equal? Most things in a netlist are considered unique objects.
My apologies, I just noticed you suggested the parent type of the EDIFCellInst
should be checked. That information is more contextual of the EDIFCellInst
and does not necessarily have bearing on what the EDIFCellInst
is. But I do agree that the documentation should be made more clear on the subject in any case.
This might not be a bug, just a documentation issue but certainly was a pitfall for me:
The minimal netlist
netlist.edf
in the attached CellInsts.zip consists of a toplevel module that instantiates two moduleslut_a_container
andlut_b_container
. These, in turn, instantiate aLUT1
and aLUT2
, respectively. Each LUT is instantiated under the namelut_inst
.After running
I receive the following results (see
rapidwright.py
in the attached ZIP):To avoid this potential pitfall (these two instances may have the same EDIFName, but are different instances), IMO either the
EDIFCellInst.equals()
operator should take the parent cell into account or the documentation should reflect this behavior.