Skip to content

[CODE HEALTH] Fix remaining misc-override-with-different-visibility warnings#4280

Merged
marcalff merged 3 commits into
open-telemetry:mainfrom
thc1006:codehealth/override-visibility-4195-part2
Jul 24, 2026
Merged

[CODE HEALTH] Fix remaining misc-override-with-different-visibility warnings#4280
marcalff merged 3 commits into
open-telemetry:mainfrom
thc1006:codehealth/override-visibility-4195-part2

Conversation

@thc1006

@thc1006 thc1006 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Changes

Completes #4195 by resolving the 5 misc-override-with-different-visibility warnings that #4215 deferred, all in the ext/http embedded server. Each override had wider visibility than its inheritance-adjusted base:

  • HttpServer privately inherits SocketCallback, so its onSocketAcceptable, onSocketReadable, onSocketWritable and onSocketClosed overrides are aligned from protected to private.
  • Reactor inherits Thread as protected, so its onThread override is aligned from public to protected.

Scope and compatibility

These overrides are dispatched only through their base interfaces (SocketCallback& and Thread::onThread), so no dynamic-dispatch call path changes. The change does narrow the source-level surface a derived class could reach (for example, calling the base implementation from a subclass) in these installed headers.

I chose this direction because the private inheritance of SocketCallback looks deliberate (the callbacks are HttpServer's internal reactor plumbing) and HttpServer is extended through its public request-handler API (e.g. operator[], setServerName) rather than by overriding the reactor callbacks; FileHttpServer is such a subclass and does not touch them. If maintainers would rather preserve that extension surface, two alternatives are: switch HttpServer to protected inheritance of SocketCallback (keeps the callbacks protected but widens the base-conversion surface), or NOLINT the four overrides. I am happy to take whichever direction you prefer.

The unrelated sendMore() helper keeps its protected visibility (an earlier revision narrowed it along with the section; that has been reverted).

Verification

  • A syntax-only build (clang-18, C++14) of file_http_server.h, which pulls in http_server.h and socket_tools.h, the FileHttpServer subclass, and an HttpServer instantiation, compiles cleanly.
  • clang-format and markdownlint 0.46.0 clean.
  • warning_limit lowered from 163/173 to 158/168. I will confirm from the clang-tidy report artifacts that misc-override-with-different-visibility reaches 0 and the totals match before treating this as ready.

Copilot AI review requested due to automatic review settings July 23, 2026 19:54
@thc1006
thc1006 requested a review from a team as a code owner July 23, 2026 19:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@thc1006
thc1006 force-pushed the codehealth/override-visibility-4195-part2 branch 2 times, most recently from 3ad6530 to 1fa7c53 Compare July 23, 2026 19:59
…arnings

Align the visibility of the SocketCallback and Thread virtual overrides in
the ext/http embedded server to match their inheritance-adjusted base
visibility, resolving the 5 misc-override-with-different-visibility warnings
that open-telemetry#4215 deferred:

- HttpServer privately inherits SocketCallback (an implementation-detail
  base), so its onSocketAcceptable, onSocketReadable, onSocketWritable and
  onSocketClosed overrides move from protected to private.
- Reactor inherits Thread as protected, so its onThread override moves from
  public to protected.

These overrides are dispatched only through their base interfaces
(SocketCallback& and Thread::onThread), so no dynamic-dispatch call path
changes. The change does narrow the source-level surface a derived class
could reach (for example calling the base implementation), but HttpServer is
extended through its public request-handler API rather than by overriding the
reactor callbacks, so this is consistent with the deliberate private
inheritance. The unrelated protected sendMore() helper keeps its visibility.

Lower the clang-tidy warning_limit to 158/168 accordingly. Fixes open-telemetry#4195.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.31%. Comparing base (de8fa51) to head (a2ce156).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4280   +/-   ##
=======================================
  Coverage   81.31%   81.31%           
=======================================
  Files         445      445           
  Lines       18859    18859           
=======================================
  Hits        15333    15333           
  Misses       3526     3526           
Files with missing lines Coverage Δ
...nclude/opentelemetry/ext/http/server/http_server.h 65.92% <ø> (ø)
...clude/opentelemetry/ext/http/server/socket_tools.h 94.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbarker dbarker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks.

@marcalff
marcalff merged commit 9d38caf into open-telemetry:main Jul 24, 2026
72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants