[Patch] perform a postmortem analysis if remove_ids fails#1799
[Patch] perform a postmortem analysis if remove_ids fails#1799tdavidcl wants to merge 6 commits intoShamrock-code:mainfrom
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances error reporting in the remove_ids method of PatchDataField by adding a post-mortem analysis block that provides diagnostic information, such as duplicate ID detection and flag sums, when an unexpected number of elements remain. The review feedback focuses on improving the robustness and idiomatic nature of this new logic, specifically by including missing standard headers, restricting the analysis to the relevant data range, utilizing standard algorithms like std::adjacent_find for duplicate detection, and ensuring type correctness in std::accumulate to avoid potential overflows.
| #include <memory> | ||
| #include <vector> |
There was a problem hiding this comment.
The code uses std::sort, std::accumulate, std::vector, and potentially smart pointers. The headers <algorithm>, <numeric>, <vector>, and <memory> should be included explicitly to ensure the code compiles across different environments.
#include <algorithm>
#include <memory>
#include <numeric>
#include <vector>|
|
||
| // post mortem analysis | ||
|
|
||
| std::vector<u32> ids_to_rem_vec = ids_to_rem.copy_to_stdvec(); |
There was a problem hiding this comment.
The remove_ids function processes only the first len elements of the ids_to_rem buffer. The postmortem analysis should be consistent with this by only analyzing those len elements. Using copy_to_stdvec() copies the entire buffer, which might contain extra data not relevant to the operation and could lead to false positives in the duplicate check.
std::vector<u32> ids_to_rem_vec = ids_to_rem.copy_to_stdvec_idx_range(0, len);| // Adgacent elements in ids_to_rem_vec should be different | ||
| for (size_t i = 1; i < ids_to_rem_vec.size(); i++) { | ||
| if (ids_to_rem_vec[i] == ids_to_rem_vec[i - 1]) { | ||
| has_duplicates = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The duplicate check can be simplified and made more readable by using std::adjacent_find instead of a manual loop. This also fixes the typo in the comment ("Adgacent" -> "Adjacent").
// Adjacent elements in ids_to_rem_vec should be different
has_duplicates = std::adjacent_find(ids_to_rem_vec.begin(), ids_to_rem_vec.end()) != ids_to_rem_vec.end();References
- Refactor logic into standard algorithms or helper functions to improve readability and maintainability.
| std::vector<u32> keep_flags_vec = keep_flag.copy_to_stdvec(); | ||
|
|
||
| // compute keep flags sum | ||
| u32 keep_flags_sum = std::accumulate(keep_flags_vec.begin(), keep_flags_vec.end(), 0); |
There was a problem hiding this comment.
The third argument to std::accumulate determines the type of the accumulator. Using 0 (a signed int) can lead to overflow if the sum of keep_flags_vec exceeds INT_MAX. Since the elements and the result are u32, the accumulator should be u32(0) or 0u to avoid signed/unsigned mismatch and ensure the accumulator type matches the data range.
u32 keep_flags_sum = std::accumulate(keep_flags_vec.begin(), keep_flags_vec.end(), u32(0));References
- When performing a scan or accumulation, if the total sum fits within a specific integer type, there is no need to use a wider type for the accumulator, as intermediate sums will not overflow.
…simple commands Copies shamenv_do from env/helpers into the build folder after setup, providing a convenience wrapper that sources activate and runs commands without manual sourcing. Assisted-by: claude-code
Workflow reportworkflow report corresponding to commit d2239be Pre-commit check reportSome failures were detected in base source checks checks. ❌ trailing-whitespace❌ check-shebang-scripts-are-executableSuggested changesDetailed changes :diff --git a/AGENTS.md b/AGENTS.md
index f8987f9f..02aba013 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -45,7 +45,7 @@ This shows the flags specific to that machine — they can vary widely.
```bash
cd build
./shamenv_do shamconfigure # alias to the correct cmake command
-./shamenv_do shammake # alias to ninja build (or make if ninja is unavailable)
+./shamenv_do shammake # alias to ninja build (or make if ninja is unavailable)Always use something line |
1 participant
No description provided.