-
Notifications
You must be signed in to change notification settings - Fork 39
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
Bugfix if-feature validate status and in enum or bits as default value notchecked #26
base: master
Are you sure you want to change the base?
Bugfix if-feature validate status and in enum or bits as default value notchecked #26
Conversation
src/yang.erl
Outdated
DefinitionName = local_name(DefinitionYangIdentifier), | ||
case map_lookup(DefinitionName, DefinitionMap) of | ||
{value, Definition} -> | ||
{Definition, Ctx0}; | ||
if Kind == 'feature' andalso Status /= undefined -> |
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.
This doesn't look quite right. I think it would be better to not do this status check here, but instead do it in chk_if_feature.
9f498ea
to
1520543
Compare
src/yang.erl
Outdated
@@ -1576,7 +1577,15 @@ chk_if_feature({_, RefArg, Pos, _}, Map0, M, Ctx0) -> | |||
fun(Ref, {Map1, Ctx2}) -> | |||
case get_feature(Ref, Pos, Map1, M, Ctx2) of | |||
{Feature, Ctx3} when is_record(Feature, feature) -> | |||
add_feature(Feature, Map1, M, Ctx3); | |||
if is_atom(Ref) -> |
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.
Why do you do this test? It checks if the reference has a prefix, so if it has a prefix you won't do this check.
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.
(sorry for the late comment; I wrote it several weeks ago, but just noticed that I forgot to hit "comment")
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 for your review, the reason for the test is to check if-feature
status only within the same module (as mentioned in RFC7950#section-7.21.2), but there are some problems with this implementation (just as you said above), I've adjusted it.
1520543
to
7c1dd40
Compare
7c1dd40
to
89d21d8
Compare
{_, IntVal} -> | ||
{ok, IntVal}; | ||
case lists:keyfind(?b2a(Val), 2, AllStmts) of |
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 think that this is not the right place to do this test - this function parses a string into an enum. In this module it is only used to parse default values, which is why this works. But the function is more generic than that - it isn't called "{parse_default_value", just "{parse". It is also exported which means that it is part of the API.
Instead, I think the right place is in "derive". The check must be different; you need to check if a default statement is present in the TypeS, and if so, find the corresponing enum statement and report an error if it has a if-feature statement.
Ditto for bits.
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 do this test according to the current logic - "Val" which is a default value is used to check whether the default value is defined, I just add a check about the default value without a if-feature
statement, so I think this may be the right position.
Follow as your tips, I reread the code about parsing the types of enumeration/bits, then debug it. If this test is placed in "derive" here,
Lines 675 to 682 in 8adf023
enumeration_type_spec_fun({derive, TypeS}, #type{base = builtin}, M, Ctx0) -> | |
{Enumstmts, Ctx1} = get_substmts(['enum'], TypeS, Ctx0), | |
parse_enums(Enumstmts, stmt_pos(TypeS), M, Ctx1, undefined); | |
enumeration_type_spec_fun({derive, TypeS}, | |
#type{type_spec = #enumeration_type_spec{ | |
enums = BaseEnums, | |
all_enums = AllBaseEnums, | |
all_enum_stmts = AllBaseStmts} = TypeSpec}, |
the default value, of course, I am not sure that I exactly understand what you mean.
No description provided.