Skip to content

Conversation

@mhasel
Copy link
Member

@mhasel mhasel commented Nov 10, 2025

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:

  1. be initialized with an explicit initializer (if present)
  2. be initialized with the 0 variant (if present)
  3. fall back to the first declared variant

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();
Copy link
Member Author

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()?

@mhasel mhasel marked this pull request as ready for review November 11, 2025 09:38
@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

❌ Patch coverage is 99.42529% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.01%. Comparing base (ce68921) to head (d34959b).
⚠️ Report is 36 commits behind head on master.

Files with missing lines Patch % Lines
src/index.rs 96.55% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mhasel mhasel requested review from ghaith and volsa November 11, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants