Fix -Wextra-semi errors in LogManagerBase.hpp#1471
Merged
bmehta001 merged 6 commits intoJun 10, 2026
Conversation
Downstream consumers that compile the public headers with -Wextra-semi -Werror (issue microsoft#1335) fail to build because several member definitions carry a redundant trailing ';': - the LogManagerBase ctor/copy-ctor/assignment/dtor written as `(){};` - every delegating method whose entire body is an LM_SAFE_CALL* macro (the macro already expands to a `{ ... }` block, so the ';' after the invocation is an extra ';' after a member function definition). Drop the redundant ';' in those 28 spots. Statement-level LM_SAFE_CALL uses inside braced bodies are intentionally left as-is, since -Wextra-semi does not flag those. No functional change. The SDK's own build uses -Wall -Werror -Wextra but not -Wextra-semi, so this never surfaced in CI. Fixes microsoft#1335. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the public LogManagerBase.hpp header to compile cleanly under Clang’s -Wextra-semi -Werror by removing redundant trailing semicolons after inline member function definitions (including functions whose entire body is provided via LM_SAFE_CALL* macros).
Changes:
- Removed extra
;after theLogManagerBasector/copy-ctor/assignment-operator/dtor inline definitions. - Removed extra
;after macro-bodied inline member function definitions where theLM_SAFE_CALL*macro expands to a{ ... }function body. - Kept statement-level
LM_SAFE_CALL*uses inside normal braced function bodies unchanged to avoid churn.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
operator= empty body / no return (LogManagerBase.hpp:145): the copy constructor and assignment operator were declared with empty bodies (the old "[not implemented]" non-copyable idiom). Copilot flagged that operator= returns LogManagerBase& with no return statement (UB if the template is instantiated and it is ever called). Convert both copy operations to = delete - the modern non-copyable idiom that Copilot recommended. The class is a static singleton utility base; verified neither the copy ctor nor operator= is referenced anywhere in lib/ or examples/. Validated the pattern compiles clean under -Wall -Wextra -Wextra-semi -Werror. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round-1 used = delete; Copilot noted that changes copy/assignment semantics in a public header (vs this PR's no-functional-change intent) and left the doc comments stale. Per Copilot's own suggestion, keep the members defined and make operator= a well-formed no-op (return *this) - this fixes the original non-returning operator= UB without altering copy semantics. Dropped the now-inaccurate "[not implemented]" comment. Validated clean under -Wall -Wextra -Wextra-semi -Werror. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(= delete) Converged on the explicit non-copyable design: delete the copy constructor and copy-assignment operator (the modern idiom, recommended in the original review). This definitively fixes the pre-existing non-returning operator= UB and removes the empty-stub ambiguity. Updated the doc comments to state the members are deleted / the class is non-copyable. Validated clean under -Wall -Wextra -Wextra-semi -Werror. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The copy-operation body (= delete vs no-op vs empty stub) drew contradictory review feedback across rounds. Per the reviewer's "keep the PR purely about removing extra semicolons" option, revert the copy constructor and operator= to their original empty bodies and original doc comments, so this PR makes NO change to copy/assignment semantics or the public API surface - it only removes the redundant trailing ';'. The pre-existing empty-body operator= is left as-is (out of scope). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
baijumeswani
approved these changes
Jun 10, 2026
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.
Summary
Remove unnecessary semicolons that could cause errors with strict compilation settings.