Skip to content

fix(globals): reject -r 0 to match must-be-positive contract (#71)#86

Merged
TimD1 merged 1 commit into
devfrom
71_D2_td_ram-zero-check
Jul 18, 2026
Merged

fix(globals): reject -r 0 to match must-be-positive contract (#71)#86
TimD1 merged 1 commit into
devfrom
71_D2_td_ram-zero-check

Conversation

@TimD1

@TimD1 TimD1 commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Summary

Globals::parse_args (src/globals.cpp) validated the -r / --max-ram value with if (this->max_ram < 0), but the error message reads "Max RAM must be positive". Message and logic disagreed: -r 0 was silently accepted despite the message claiming the value must be positive.

How max_ram is used downstream

I checked whether 0 is a meaningful sentinel (e.g. "unlimited") before choosing a fix. It is not:

  • src/globals.cpp:439ram_steps.push_back(this->max_ram / threads); with max_ram == 0 every RAM step is 0.
  • src/dist.cpp:790while (total_ram < g.max_ram && thread_step >= 0); total_ram starts at 0, so 0 < 0 is false and the "not enough work for all threads" branch schedules zero threads, silently dropping superclusters.
  • src/cluster.cpp:607if (mem_gb > g.max_ram); with max_ram == 0 every supercluster "exceeds" RAM and triggers the warning path.

There is no "0 means unlimited" semantics anywhere; 0 simply breaks scheduling.

Fix

Changed the check to if (this->max_ram <= 0) so the validation matches the "must be positive" contract stated in the message. This is the minimal change and the option consistent with actual usage (rather than softening the message, since 0 is not a valid value).

Compile check

cd src && g++ -c -g -O1 -Wall -Wextra -std=c++17 globals.cpp -o /tmp/probe_71.o — clean, no warnings.

Fixes #71
Sub-issue of #45; documented in docs/D2_vcfdist-v3-unit-tests.md §6 defect #13.

@TimD1
TimD1 merged commit 15a5560 into dev Jul 18, 2026
1 check passed
@TimD1
TimD1 deleted the 71_D2_td_ram-zero-check branch July 18, 2026 03:08
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.

1 participant