Skip to content

Complete warning-free step 2.3A cleanup - #55

Merged
egull merged 7 commits into
modernize/step2from
modernize/step2.3a
Sep 24, 2026
Merged

egull merged 7 commits into
modernize/step2from
modernize/step2.3a

Conversation

@egull

@egull egull commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • eliminate all Maxent-owned compiler warnings and enable MAXENT_WERROR in the dev preset
  • make back-continuation size validation unconditional, robustly parse both three- and five-column text input at EOF, and remove unreachable/dead compatibility code
  • add regression coverage for trailing blank lines and incomplete final records, and document completion of migration step 2.3A
  • preserve and explicitly test the unconditional high-frequency diagnostic for both diagonal-error and covariance input

Verification

  • warning-free AppleClang build with -Wall -Wextra -Wpedantic -Werror
  • warning-free LAPACK build
  • Release suite with full regressions: 44/44 tests passed
  • fast and full numerical regression references unchanged
  • git diff --check passes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The regression test does not expose the failed-extraction EOF path and needs strengthening.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Completes Maxent migration step 2.3A by removing warnings, fixing EOF parsing and debug validation, and documenting completion.

Changes:

  • Enables warnings-as-errors in the development preset.
  • Cleans warning-producing and obsolete code.
  • Adds regression coverage and updates migration documentation.
File Reviewed changes
test/​paramsTest.cpp Adds trailing-blank-line input coverage.
test/​backcontTest.cpp Tests debug size validation.
src/​maxent.cpp Updates origin-name handling.
src/​maxent_simulation.cpp Fixes initialization and index types.
src/​maxent_params.cpp Fixes EOF parsing and indexing.
src/​maxent_matrix_def.hpp Removes obsolete configuration inclusion.
src/​maxent_kernel.cpp Removes unreachable branches.
src/​maxent_helper.cpp Cleans index types and dead code.
src/​maxent_backcont.hpp Removes obsolete inclusion.
src/​maxent_backcont.cpp Corrects the debug size guard.
src/​eigen_lapack.hpp Makes the helper inline.
src/​eigen_hdf5.hpp Makes helpers inline and fixes indexing.
legendre_convert/​legendre_convert.cpp Removes an unused variable.
CMakePresets.json Enables warnings-as-errors in the dev preset.
ALPS3_MIGRATION_PLAN.md Documents completion of step 2.3A.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/paramsTest.cpp

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The high-frequency diagnostic check changes behavior outside the covariance-only path and needs correction or intentional updated coverage.

Review effort: Lite
Findings: None

Resolved since last review (1)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Reconcile the migration log’s 43/43 Release-test count with the verified 42/42 result.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Reconcile inconsistent Release test counts

ALPS3_MIGRATION_PLAN.md:686

This migration-log entry reports 43/43 Release tests, but the PR verification says the Release suite passed 42/42. Please reconcile the count (or explain which additional test is included) so the completion record is consistent with the verified result.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical warning-cleanup and moderate covariance-diagnostic coverage findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread src/maxent_helper.cpp
Comment thread test/paramsTest.cpp

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Add the requested five-column text parsing regression and remove the unnecessary compatibility guard.

Review effort: Lite
Findings: None

Resolved since last review (2)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

src/maxent_params.cpp can silently accept a malformed partial or non-numeric trailing record.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)

Comment thread src/maxent_params.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (1)

@egull
egull merged commit a57f150 into modernize/step2 Sep 24, 2026
1 check passed
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