Skip to content

Conversation

@Roll249
Copy link
Contributor

@Roll249 Roll249 commented May 31, 2025

No description provided.

Roll249 added 2 commits May 31, 2025 06:50
…cityInBytes() for Linux, macOS, and Windows. Falls back to exception if not available.
@Roll249
Copy link
Contributor Author

Roll249 commented May 31, 2025

can you check that @TysonRayJones

@TysonRayJones
Copy link
Member

Hi there,

That's a great effort! Alas there are a few issues:

  1. Could you please open a new PR directing to devel branch rather than main? Merely retargeting this PR will be insufficient because you've forked from main. Apologies for not having made this clear elsewhere!
  2. You've accidentally committed some of the build folder. This should stay local/temporary to your machine, and has broken the automatic CI. I personally find GUIs like Github Desktop helpful to avoid accidental diffs.
  3. Despite some outdated posts online, parsing proc/meminfo directly is re-inventing the wheel at this point. C packages like sysinfo are standard to Linux C++.
  4. Could you move the header includes to the top of the file, beneath the existing includes? This means including compiler guards there too, of course, but makes it much easier to quickly see dependencies
  5. Some newlines and indentation would go a long way for reader clarity!

@TysonRayJones
Copy link
Member

TysonRayJones commented May 31, 2025

Closing as a duplicate of #636

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