Skip to content

Replace O(n) List.length comparisons with O(1) List.is_empty#1594

Merged
WardBrian merged 5 commits intostan-dev:masterfrom
ishaan-arora-1:fix/use-list-is-empty
Feb 23, 2026
Merged

Replace O(n) List.length comparisons with O(1) List.is_empty#1594
WardBrian merged 5 commits intostan-dev:masterfrom
ishaan-arora-1:fix/use-list-is-empty

Conversation

@ishaan-arora-1
Copy link
Copy Markdown
Contributor

@ishaan-arora-1 ishaan-arora-1 commented Feb 22, 2026

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here: N/A
    • OR, no user-facing changes were made

Summary

Fixes #1591

Replace List.length l = 0 and List.length l > 0 with the idiomatic List.is_empty / not (List.is_empty ...) in two files:

  • src/frontend/Warnings.ml: List.length warnings > 0not (List.is_empty warnings)
  • src/analysis_and_optimization/Optimize.ml: 3 occurrences of List.length ... = 0List.is_empty ...

List.is_empty is O(1) while List.length traverses the entire list. The codebase already uses List.is_empty in 26+ other locations, making these remaining usages inconsistent with the established convention.

Release notes

Internal code quality improvement: replace O(n) List.length empty-checks with O(1) List.is_empty for consistency and minor performance improvement.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

AI Policy

This fix was very straightforwad, hence no AI involvement

List.length traverses the entire list (O(n)) just to compare against
zero. List.is_empty is a constant-time check and is already the
established idiom elsewhere in the codebase.

Changed in:
- Warnings.ml: List.length warnings > 0 → not (List.is_empty warnings)
- Optimize.ml: List.length l' = 0 → List.is_empty l' (3 occurrences)
Warnings.ml does not open Core, so List refers to the stdlib List
module (shadowed by -open Frontend). Use the simpler `warnings <> []`
pattern which works in any module context.
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 22, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.34%. Comparing base (0121e19) to head (cdab8c1).

Files with missing lines Patch % Lines
src/analysis_and_optimization/Optimize.ml 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1594   +/-   ##
=======================================
  Coverage   90.34%   90.34%           
=======================================
  Files          65       65           
  Lines       10027    10027           
=======================================
  Hits         9059     9059           
  Misses        968      968           
Files with missing lines Coverage Δ
src/frontend/Warnings.ml 83.33% <100.00%> (ø)
src/analysis_and_optimization/Optimize.ml 92.44% <66.66%> (ø)
🚀 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.

Comment thread src/frontend/Warnings.ml Outdated
@WardBrian
Copy link
Copy Markdown
Member

Ah, I see the issue in Warning.ml. We probably don't want to open Core, since this brings in a lot of other stuff into scope. The smallest change is probably just using not (Core.List.is_empty ...

@ishaan-arora-1
Copy link
Copy Markdown
Contributor Author

Ah, I see the issue in Warning.ml. We probably don't want to open Core, since this brings in a lot of other stuff into scope. The smallest change is probably just using not (Core.List.is_empty ...

oh alright, understood, ill try that

Copy link
Copy Markdown
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Thanks

@WardBrian WardBrian merged commit 8e1358f into stan-dev:master Feb 23, 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.

Replace O(n) List.length comparisons with O(1) List.is_empty

2 participants