fix(cie): address misc code quality issues#51
Merged
atsushi421 merged 2 commits intomainfrom Apr 22, 2026
Merged
Conversation
- Reject unknown scheduling policies in issue_syscalls() instead of silently succeeding - Remove trailing comma when writing cpuset.cpus - Add inline to sched_setattr() in header to prevent ODR violation - Remove stale CARGO_TERM_COLOR env var from CI workflow - Zero-initialize ThreadConfig members (priority, runtime, period, deadline) Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
- Add thread_id to unknown-policy error message for consistency - Validate policy string at YAML load time and fail fast on typos Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves robustness and code quality in the CIE thread configurator by tightening scheduling-policy validation/handling, fixing cpuset formatting, addressing a potential ODR issue in a public header, and cleaning up CI configuration.
Changes:
- Validate scheduling policy strings at YAML load time and reject unknown policies in
issue_syscalls(). - Fix
cpuset.cpusformatting to avoid a trailing comma. - Make
sched_setattr()inline, remove stale CI env var, and default-initializeThreadConfigfields.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cie_thread_configurator/src/thread_configurator_node.cpp |
Adds policy validation, fixes cpuset writing, and rejects unknown policies at syscall time. |
cie_thread_configurator/include/cie_thread_configurator/thread_configurator_node.hpp |
Default-initializes ThreadConfig scheduling fields to avoid uninitialized reads. |
cie_thread_configurator/include/cie_thread_configurator/sched_deadline.hpp |
Marks sched_setattr() as inline to prevent ODR violations across TUs. |
.github/workflows/build-and-test.yaml |
Removes a Rust-specific env var from the C++ CI workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Description
Fix several code quality issues found during repository review:
issue_syscalls()now returnsfalseand logs an error when an unrecognized policy string is encountered, instead of silently succeeding and only applying CPU affinity. Additionally, the policy string is validated at YAML load time so that typos (e.g.SCHED_OTEHR) cause an immediatestd::runtime_errorat startup rather than a deferred runtime failure.set_affinity_by_cgroup()now writes"0,1,2"instead of"0,1,2,"for canonical cpuset format.inlinetosched_setattr()in header: Prevents ODR violation ifsched_deadline.hppis included from multiple translation units. Currently only one TU includes it, but the header is publicly installed.CARGO_TERM_COLORenv var: Rust-specific environment variable copied into the C++ CI workflow.ThreadConfigmembers:priority,runtime,period, anddeadlinenow have default initializers to avoid uninitialized reads if access patterns change in the future.Related links
How was this PR tested?
Notes for reviewers