Code review fixes: safety, performance, and correctness#38
Merged
Conversation
…ormat - Remove local format_compact lambda from imgui_system_panel_view.cpp and use the shared pex::format_bytes() utility instead - Replace std::ostringstream in format_bytes() with std::format for better performance in hot rendering paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The sun_path field in sockaddr_un has a platform-specific size limit (~104-108 bytes). If XDG_RUNTIME_DIR produces a path exceeding this limit, fall back to the /tmp-based path which always fits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents silently swallowing non-standard exceptions like std::bad_alloc while still handling expected filesystem_error, invalid_argument, and out_of_range exceptions from /proc iteration and string parsing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace std::map with std::unordered_map for process_map, previous_cpu_times_, and tree-building temporaries. These maps are keyed by int PIDs and only need point lookups, making O(1) hash lookups faster than O(log n) tree lookups on the hot path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace linear scan with std::upper_bound for O(log n) lookups when mapping thread instruction pointers to library names. The address_map from /proc/pid/maps is already sorted by start address. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace per-call kstat_open()/kstat_close() with a cached handle that persists across calls. Uses kstat_chain_update() to keep the chain current and reopens on failure. Eliminates ~5 kstat_open() calls per data collection cycle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Before sending signals during tree kill, re-verify that each PID still refers to the original process by comparing start times. This prevents accidentally killing an unrelated process if the PID was recycled between collection and signal delivery, matching the existing Linux implementation's safety check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add comments in main.cpp and main_tui.cpp explaining why declaration order matters (C++ destroys locals in reverse order, so providers must be declared before DataStore). Also document the lifetime contract on DataStore's constructor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Solaris pkg install returns exit code 4 when a package is already
installed ("no updates necessary"). Add || true to each pkg install
in the prepare step to prevent the VM's shell from aborting on this
non-error condition.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CMake's FetchContent needs git to clone GLFW. The Solaris VM image previously included git but the current image does not. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Solaris's default make is dmake, which doesn't support GNU make's -j flag syntax. Use CMAKE_MAKE_PROGRAM which resolves to gmake on Solaris (since CMake uses GNU make as its generator). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
format_compactlambda with sharedformat_bytes()utility in ImGui system panel, and replaceostringstreamwithstd::formatfor better performancesockaddr_un::sun_pathlimit before usingXDG_RUNTIME_DIR, with/tmpfallbackcatch(...)tocatch(const std::exception&)across 8 platform files to avoid silently swallowingstd::bad_allocunordered_mapfor PID-keyed maps in DataStore and UI (O(1) vs O(log n) lookups on the hot path)std::upper_bound) for thread library detection in address mapkstat_chain_update()instead of open/close per callmain.cpp/main_tui.cppFindings from the code review in
code-review-4.6.md. One finding (#2 — removeProcessNode::is_expanded) was investigated and found to be inaccurate (the field is used by ImGui), so it was skipped.Test plan
pexandpexc)pexandpexcon Linux, confirm system panel, process list, and kill functionality work🤖 Generated with Claude Code