Michal/diameter/fix-inheriting-of-indirect-inherits/otp-19626 #10149
Conversation
CT Test Results 2 files 36 suites 12m 20s ⏱️ Results for commit 08fb796. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
There was a problem hiding this comment.
Pull Request Overview
This PR implements an indirect_inherits feature for the Diameter dictionary compiler that enables automatic recursive inheritance resolution. The feature ensures that when a dictionary inherits from another dictionary, all ancestors in the inheritance chain are automatically considered during code generation, eliminating the need to explicitly list all parent dictionaries.
- Adds new
indirect_inheritsoption to the diameter compiler - Implements automatic resolution of inheritance chains with proper handling of AVPs, enums, and vendor IDs
- Adds comprehensive test suites to verify inheritance behavior
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/diameter/test/modules.mk | Adds new test suites to the build configuration |
| lib/diameter/test/diameter_indirect_inherits_SUITE.erl | Comprehensive test suite for indirect inheritance functionality |
| lib/diameter/test/diameter_codegen_SUITE.erl | Test suite for code generation aspects of indirect inheritance |
| lib/diameter/test/diameter_compiler_SUITE.erl | Updates existing compiler tests to support indirect inherits |
| lib/diameter/src/compiler/diameter_make.erl | Adds documentation for the new indirect_inherits option |
| lib/diameter/src/compiler/diameter_dict_util.erl | Core implementation of inheritance chain resolution logic |
| lib/diameter/src/compiler/diameter_codegen.erl | Updates code generation to handle inherited enums properly |
| lib/diameter/doc/references/diameterc_cmd.md | Documents the new --indirect-inherits command line option |
| lib/diameter/doc/references/diameter_dict.md | Comprehensive documentation of inheritance behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| end_per_suite/1, | ||
| init_per_testcase/2, | ||
| end_per_testcase/2, | ||
|
|
There was a problem hiding this comment.
Remove the trailing whitespace on this line.
Copilot uses AI. Check for mistakes.
| end_per_suite/1, | ||
| init_per_testcase/2, | ||
| end_per_testcase/2, | ||
|
|
There was a problem hiding this comment.
Remove the trailing whitespace on this line.
Copilot uses AI. Check for mistakes.
| verify_multiple_limited_imports_same_file(_) -> | ||
| %% This test checks that when you inherit same avp twice you get avp_already_defined error. | ||
| DictA = ?AVP_DICT_A, | ||
| DictB = ?AVP_DICT_B(["@inherits diameter_test_a AAA", "@inherits diameter_test_a AAA"]), |
There was a problem hiding this comment.
[nitpick] Consider extracting the duplicate inherit statement into a variable to make the test intent clearer and reduce duplication.
Copilot uses AI. Check for mistakes.
| % inherited_modules/2 | ||
| % Returns list of inherited modules, without returning module in which avp is defined |
There was a problem hiding this comment.
The comment style is inconsistent - use %% for function documentation comments to match the rest of the file.
| % inherited_modules/2 | |
| % Returns list of inherited modules, without returning module in which avp is defined | |
| %% inherited_modules/2 | |
| %% Returns list of inherited modules, without returning module in which avp is defined |
Copilot uses AI. Check for mistakes.
| NestedInherits = lists:flatmap(fun([]) -> | ||
| []; | ||
| ({Mod, _}) -> | ||
| get_all_inherits_from_module(dict(?A(Mod))) | ||
| end, Inherits), |
There was a problem hiding this comment.
[nitpick] The pattern matching for empty list ([]) -> [] could be simplified by filtering out empty lists before the flatmap operation for better readability.
| NestedInherits = lists:flatmap(fun([]) -> | |
| []; | |
| ({Mod, _}) -> | |
| get_all_inherits_from_module(dict(?A(Mod))) | |
| end, Inherits), | |
| NestedInherits = lists:flatmap( | |
| fun({Mod, _}) -> | |
| get_all_inherits_from_module(dict(?A(Mod))) | |
| end, | |
| [I || I <- Inherits, I =/= []] | |
| ), |
Copilot uses AI. Check for mistakes.
Mikaka27
added
team:PS
Fixes #8235