Surfaced by a kluster.ai review on PR #17 (see inline comment).
The observation
string_to_bytes in src/eatmemory.c is conceptually a pure conversion: take a string like "10M" and produce a byte count. The % branch breaks that abstraction by reaching out to the OS for get_system_memory_stats() to resolve "X% of available memory" into an absolute byte count.
} else if (unit == '%') {
struct system_memory_stats memory_stats;
get_system_memory_stats(&memory_stats);
if (!memory_stats.supported) {
*error = EM_ERROR_PARSE_INVALID_UNIT;
return 0;
}
numerator = memory_stats.free;
denominator = 100;
}
Why this is a design smell, not a bug
The function does the right thing today. The objections are:
- Couples parser to runtime environment. A pure parser is easier to reason about, easier to unit test against fixed inputs, and doesn't need to know the OS exists.
- Mixes layers. Parsing user input ("transport-layer logic") and resolving a percentage against current free memory ("business logic") are different concerns currently fused.
- Hardens testability. With the current shape, the only way to test the
% path is to actually have a working get_system_memory_stats for the host platform; mocking it requires intrusion.
Possible shapes
string_to_bytestakes asystem_memory_stats*parameter (caller provides; can be NULL on platforms where it's not supported, in which case%` is rejected).
Surfaced by a kluster.ai review on PR #17 (see inline comment).
The observation
string_to_bytesinsrc/eatmemory.cis conceptually a pure conversion: take a string like"10M"and produce a byte count. The%branch breaks that abstraction by reaching out to the OS forget_system_memory_stats()to resolve "X% of available memory" into an absolute byte count.Why this is a design smell, not a bug
The function does the right thing today. The objections are:
%path is to actually have a workingget_system_memory_statsfor the host platform; mocking it requires intrusion.Possible shapes
string_to_bytes
takes asystem_memory_stats*parameter (caller provides; can be NULL on platforms where it's not supported, in which case%` is rejected).