-
Notifications
You must be signed in to change notification settings - Fork 16
fix: fix error on Archlinux build #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds explicit handling for nodiscard file-open results and fixes a cleanup call in the Wayland/Treeland connector to avoid Archlinux build errors and improve error checking. Sequence diagram for TreelandConnector setPrivateObject cleanup behaviorsequenceDiagram
participant Client
participant TreelandConnector
participant TreelandDDM as treeland_ddm
Client->>TreelandConnector: setPrivateObject(new_ddm)
alt existing private object
TreelandConnector->>TreelandDDM: treeland_ddm_destroy(m_ddm)
TreelandConnector-->>TreelandConnector: m_ddm = nullptr
end
TreelandConnector-->>TreelandConnector: m_ddm = new_ddm
Class diagram for TreelandConnector updated ownership of treeland_ddmclassDiagram
class TreelandConnector {
- treeland_ddm* m_ddm
+ bool isConnected()
+ void setPrivateObject(treeland_ddm* ddm)
}
class treeland_ddm {
}
TreelandConnector ..> treeland_ddm : manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Using
Q_ASSERT(ok)afterfile.open(...)changes the behavior to abort in debug builds when the file cannot be opened; if this failure is expected/possible in normal operation, consider explicitly handling the error (e.g., early return or fallback path) instead of asserting, and use(void)file.open(...)or[[maybe_unused]]onokto satisfy thenodiscardwarning. - In
ConfigReader, opening the file for read/write now relies solely onQ_ASSERT(ok)for failure handling; if opening the config file can legitimately fail (e.g., missing or unreadable file), it may be more robust to propagate or log the error instead of only asserting in debug builds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `Q_ASSERT(ok)` after `file.open(...)` changes the behavior to abort in debug builds when the file cannot be opened; if this failure is expected/possible in normal operation, consider explicitly handling the error (e.g., early return or fallback path) instead of asserting, and use `(void)file.open(...)` or `[[maybe_unused]]` on `ok` to satisfy the `nodiscard` warning.
- In `ConfigReader`, opening the file for read/write now relies solely on `Q_ASSERT(ok)` for failure handling; if opening the config file can legitimately fail (e.g., missing or unreadable file), it may be more robust to propagate or log the error instead of only asserting in debug builds.
## Individual Comments
### Comment 1
<location> `src/common/MessageHandler.h:77-79` </location>
<code_context>
if (!file.isOpen() && !isatty(STDERR_FILENO)) {
- if (!file.open(QFile::Append | QFile::WriteOnly))
- file.open(QFile::Truncate | QFile::WriteOnly);
+ if (!file.open(QFile::Append | QFile::WriteOnly)) {
+ bool ok = file.open(QFile::Truncate | QFile::WriteOnly);
+ Q_ASSERT(ok);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Asserting on `file.open` here conflicts with the intended fallback behavior for non-writable log locations.
Because we intentionally fall back to a writable location when the primary log path can’t be opened, asserting on the truncate open short-circuits that behavior: debug builds will abort instead of reaching the `if (!file.isOpen()) { ... }` fallback when the system log path is unwritable. That’s a regression for valid environments like read‑only setups.
To preserve the documented behavior while still catching unexpected errors, consider either removing this `Q_ASSERT`, only asserting after the fallback also fails, or replacing it with a logged warning. The same applies to the later `Q_ASSERT(ok)` when `file` points to `~/.local/share/.../ddm.log`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Those are mostly nodiscard errors. Simply add some assertions.
Those are mostly nodiscard errors. Simply add some assertions.
Summary by Sourcery
Address file handling and Wayland connector cleanup issues uncovered during Arch Linux builds.
Bug Fixes: