-
Notifications
You must be signed in to change notification settings - Fork 65
feat: add typed enums and allow type-specifier to be suffixed #1544
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
base: master
Are you sure you want to change the base?
Conversation
Add support for typed enums with explicit integer type specifications, supporting both IEC 61131-3 standard syntax (TYPE NAME : TYPE (...)) and Codesys syntax (TYPE NAME : (...) TYPE).
| let (mut index, unresolvables) = plc::resolver::const_evaluator::evaluate_constants(global_index); | ||
|
|
||
| // Fix up enum defaults after constants are resolved | ||
| index.finalize_enum_defaults(); |
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.
Design Question: Should finalize_enum_defaults() be integrated into evaluate_constants()?
Currently, finalize_enum_defaults() is called separately after evaluate_constants() in three places:
- Main pipeline (pipelines.rs)
- Test utilities (test_utils.rs, 2 locations)
Arguments for keeping them separate (current approach):
- Clear separation of concerns: Constant evaluation and enum initialization are conceptually distinct operations
- Modularity: Each function has a single, well-defined responsibility
- Explicit pipeline: Makes it obvious what post-resolution steps are needed
Arguments for integrating into evaluate_constants():
- Strong dependency:
finalize_enum_defaults()fundamentally requires resolved constants and cannot work without them - Simpler API: Single function call instead of remembering to call both in sequence
- Maintenance: Reduces chance of forgetting this pass in new code paths
- Logical cohesion: Both operations finalize constant-related initialization
Should we consider renaming to something like resolve_constants_and_initializers() or updating the evaluate_constants() doc comment to clarify it also handles dependent initialization steps and add this pass to evaluate_constants()?
7327160 to
b676e54
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1544 +/- ##
==========================================
+ Coverage 94.03% 95.01% +0.98%
==========================================
Files 174 175 +1
Lines 58611 56206 -2405
==========================================
- Hits 55112 53406 -1706
+ Misses 3499 2800 -699 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add support for typed enums with explicit integer type specifications, supporting both IEC 61131-3 standard syntax (TYPE NAME : TYPE (...)) and Codesys syntax (TYPE NAME : (...) TYPE).
Additionally, enums are no longer blanket-zero-initialized but rather they will: