-
Notifications
You must be signed in to change notification settings - Fork 114
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
Enforce Network Keyword Consistency #3885
Conversation
The standard network model (GRUPNET) prohibits any extended network model keywords and vice versa. Similarly, the *PROP keywords require the 'NETWORK' keyword, and the NODEPROP keyword also requires the 'BRANPROP' keyword.
jenkins build this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, should be merged.
@@ -68,6 +75,10 @@ BOOST_AUTO_TEST_CASE(Branch) { | |||
|
|||
BOOST_AUTO_TEST_CASE(INVALID_DOWNTREE_NODE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what this and the next was supposed to test, but after the change won't both just throw due to both NETWORK and GRUPNET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but after the change won't both just throw due to both
NETWORK
andGRUPNET
?
I don't see GRUPNET
here. There is GRUPTREE
but no GRUPNET
as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, thanks for pointing it out.
@@ -3,6 +3,8 @@ | |||
"sections": [ | |||
"SCHEDULE" | |||
], | |||
"requires": ["NETWORK", "BRANPROP"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not really aware of this feature, have we had it all the time?!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we had [
requires
/prohibits
] all the time?!?
The feature was introduced in October 2020 (PR #1996), but it's not particularly widely deployed. Most of its usage is in the table input code, and especially in the saturation functions (SOF3
etc.) to reject decks using saturation function tables from both families.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your review. I've tried to address your comments.
@@ -68,6 +75,10 @@ BOOST_AUTO_TEST_CASE(Branch) { | |||
|
|||
BOOST_AUTO_TEST_CASE(INVALID_DOWNTREE_NODE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but after the change won't both just throw due to both
NETWORK
andGRUPNET
?
I don't see GRUPNET
here. There is GRUPTREE
but no GRUPNET
as far as I can tell.
@@ -3,6 +3,8 @@ | |||
"sections": [ | |||
"SCHEDULE" | |||
], | |||
"requires": ["NETWORK", "BRANPROP"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we had [
requires
/prohibits
] all the time?!?
The feature was introduced in October 2020 (PR #1996), but it's not particularly widely deployed. Most of its usage is in the table input code, and especially in the saturation functions (SOF3
etc.) to reject decks using saturation function tables from both families.
PR approved and build check is green. I'll merge into master. |
The standard network model (
GRUPNET
) prohibits any extended network model keywords and vice versa. Similarly, the*PROP
keywords require theNETWORK
keyword, and theNODEPROP
keyword also requires theBRANPROP
keyword.