misc cleanup and bug fixes#541
Merged
Merged
Conversation
Reviewer's GuideRefactors several subsystems for safer lifetime/ownership semantics, introduces reusable circular index helpers, and tightens null/exception handling across UI, map storage, proxy, parser, and utility code while removing some dead code and minor build ordering issues. Sequence diagram for Proxy::UserSocket null-safe operationssequenceDiagram
participant Proxy
participant UserSocket
participant AbstractSocket
participant UserSocketOutputs as Outputs
Proxy->>Proxy: allocUserSocket()
Proxy->>UserSocket: UserSocket(get_socket_lambda, parent, outputs)
UserSocket->>AbstractSocket: getSocket()
AbstractSocket-->>UserSocket: sig_disconnected
UserSocket->>Outputs: onDisconnected()
AbstractSocket-->>UserSocket: readyRead
UserSocket->>Outputs: onReadyRead()
UserSocket->>UserSocket: disconnectFromHost()
UserSocket->>AbstractSocket: getSocket()
alt [socket is null]
UserSocket->>UserSocket: qWarning("non-existent user socket")
else [socket exists]
UserSocket->>AbstractSocket: flush()
UserSocket->>AbstractSocket: disconnectFromHost()
end
UserSocket->>UserSocket: sendToSocket(bytes)
UserSocket->>AbstractSocket: getSocket()
alt [socket is null]
UserSocket->>UserSocket: qWarning("non-existent user socket")
else [socket exists]
UserSocket->>AbstractSocket: isConnected()
alt [!isConnected]
UserSocket->>UserSocket: qWarning("closed user socket")
else [connected]
UserSocket->>AbstractSocket: write(bytes.getQByteArray())
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In MainWindow’s Log Panel setup lambda,
new QTextBrowser(m_dockDialogLog)still uses the (null) member as parent instead of the freshly createddock, which will break the parent/ownership chain; this should benew QTextBrowser(dock)andm_dockDialogLogupdated afterward as you already do. - Changing
AbstractParser::showSyntaxto throwstd::invalid_argumenton a nullrestpointer is a behavior change from the priorassert/empty-string handling; verify that all existing callers never pass null or consider preserving the old behavior to avoid unexpected exceptions in release builds. - The new manual
delete m_proxyinConnectionListener::~ConnectionListenersubtly changes ownership semantics ofm_proxy; it would be safer and clearer to convert this to an owning smart pointer (as hinted in the comment) and rely on member destruction order rather than a raw-pointer delete in the destructor.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In MainWindow’s Log Panel setup lambda, `new QTextBrowser(m_dockDialogLog)` still uses the (null) member as parent instead of the freshly created `dock`, which will break the parent/ownership chain; this should be `new QTextBrowser(dock)` and `m_dockDialogLog` updated afterward as you already do.
- Changing `AbstractParser::showSyntax` to throw `std::invalid_argument` on a null `rest` pointer is a behavior change from the prior `assert`/empty-string handling; verify that all existing callers never pass null or consider preserving the old behavior to avoid unexpected exceptions in release builds.
- The new manual `delete m_proxy` in `ConnectionListener::~ConnectionListener` subtly changes ownership semantics of `m_proxy`; it would be safer and clearer to convert this to an owning smart pointer (as hinted in the comment) and rely on member destruction order rather than a raw-pointer delete in the destructor.
## Individual Comments
### Comment 1
<location path="src/mapstorage/MapDestination.cpp" line_range="73-82" />
<code_context>
+ }
+}
+
+MapDestination::~MapDestination()
+{
+ // finalize() calls FileSaver::close() which can throw on rename failure;
+ // catch here to avoid std::terminate() if this destructor runs during
+ // stack unwinding from a prior finalize() error.
+ try {
+ finalize();
+ } catch (const std::exception &ex) {
+ qWarning() << "MapDestination::~MapDestination() caught exception:" << ex.what();
+ } catch (...) {
+ qWarning() << "MapDestination::~MapDestination() caught unknown exception";
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling finalize() unconditionally in the destructor can double-close the FileSaver and change exception behavior.
With this change, destruction now implicitly calls `finalize()`, which calls `FileSaver::close()` on native platforms. If callers still invoke `finalize()` explicitly (e.g. to control when errors surface), `close()` may be executed twice on the same `FileSaver`, which may not be safe and could hide failures. It also alters error semantics: exceptions that used to propagate from an explicit `finalize()` may now be caught in the destructor instead.
Consider either making `finalize()` idempotent via a `m_finalized` flag, or reverting to explicit finalization and documenting that protocol instead of calling `finalize()` from the destructor.
</issue_to_address>
### Comment 2
<location path="src/proxy/connectionlistener.cpp" line_range="54-63" />
<code_context>
{}
-ConnectionListener::~ConnectionListener() = default;
+ConnectionListener::~ConnectionListener()
+{
+ // Note: We're the owner of the proxy, so we're allowed to delete it.
+ // Deleting the proxy here somehow prevents the proxy's destructor from triggering
+ // use-after-free undefined behavior when the proxy tries to send a goodbye message
+ // to the user.
+ //
+ // Another option would be to make m_proxy an owning pointer (e.g. unique_ptr or QScopedPointer),
+ // and then make sure it's declared last so it'll be deleted first, and then make this dtor
+ // a default dtor again.
+ if (Proxy *const pProxy = m_proxy) {
+ MMLOG() << "ConnectionListener is deleting the proxy...";
+ delete pProxy;
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Manually deleting m_proxy in the destructor is risky unless ConnectionListener is the sole owner.
Because `m_proxy` is a raw pointer, other parts of the code may still treat it as non-owning (e.g., via QObject parenting or separate ownership). Deleting it unconditionally here risks double-deletion or a dangling pointer during teardown.
If ConnectionListener is the true owner, please model that explicitly with `std::unique_ptr<Proxy>` or `QScopedPointer<Proxy>` and order the members so the proxy is destroyed before its users. If ownership is shared/unclear, avoid deleting it here and instead disconnect and null out `m_proxy` while relying on the actual owner to destroy it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+73
to
+82
| MapDestination::~MapDestination() | ||
| { | ||
| // finalize() calls FileSaver::close() which can throw on rename failure; | ||
| // catch here to avoid std::terminate() if this destructor runs during | ||
| // stack unwinding from a prior finalize() error. | ||
| try { | ||
| finalize(); | ||
| } catch (const std::exception &ex) { | ||
| qWarning() << "MapDestination::~MapDestination() caught exception:" << ex.what(); | ||
| } catch (...) { |
There was a problem hiding this comment.
issue (bug_risk): Calling finalize() unconditionally in the destructor can double-close the FileSaver and change exception behavior.
With this change, destruction now implicitly calls finalize(), which calls FileSaver::close() on native platforms. If callers still invoke finalize() explicitly (e.g. to control when errors surface), close() may be executed twice on the same FileSaver, which may not be safe and could hide failures. It also alters error semantics: exceptions that used to propagate from an explicit finalize() may now be caught in the destructor instead.
Consider either making finalize() idempotent via a m_finalized flag, or reverting to explicit finalization and documenting that protocol instead of calling finalize() from the destructor.
Comment on lines
+54
to
+63
| ConnectionListener::~ConnectionListener() | ||
| { | ||
| // Note: We're the owner of the proxy, so we're allowed to delete it. | ||
| // Deleting the proxy here somehow prevents the proxy's destructor from triggering | ||
| // use-after-free undefined behavior when the proxy tries to send a goodbye message | ||
| // to the user. | ||
| // | ||
| // Another option would be to make m_proxy an owning pointer (e.g. unique_ptr or QScopedPointer), | ||
| // and then make sure it's declared last so it'll be deleted first, and then make this dtor | ||
| // a default dtor again. |
There was a problem hiding this comment.
issue (bug_risk): Manually deleting m_proxy in the destructor is risky unless ConnectionListener is the sole owner.
Because m_proxy is a raw pointer, other parts of the code may still treat it as non-owning (e.g., via QObject parenting or separate ownership). Deleting it unconditionally here risks double-deletion or a dangling pointer during teardown.
If ConnectionListener is the true owner, please model that explicitly with std::unique_ptr<Proxy> or QScopedPointer<Proxy> and order the members so the proxy is destroyed before its users. If ownership is shared/unclear, avoid deleting it here and instead disconnect and null out m_proxy while relying on the actual owner to destroy it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #541 +/- ##
==========================================
- Coverage 25.69% 25.66% -0.04%
==========================================
Files 521 521
Lines 43187 43249 +62
Branches 4706 4708 +2
==========================================
- Hits 11099 11098 -1
- Misses 32088 32151 +63 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 by Sourcery
Refine object ownership and construction patterns, harden error handling and safety checks, and clean up unused or redundant logic across the UI, map storage, networking proxy, parsing, and OpenGL subsystems.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: