Skip to content

Conversation

@gophergogo
Copy link
Collaborator

No description provided.

gophergogo added 7 commits October 5, 2025 15:39
JSON-RPC allows string or number for request IDs. Using int64_t instead
of int prevents overflow when generating numeric request IDs from
uint64_t counters.

Changes:
- Update RequestId type from variant<std::string, int> to variant<std::string, int64_t>
- Update ProgressToken type to use int64_t for consistency
- Add factory function for int64_t request IDs
- Update all usages of holds_alternative<int> to holds_alternative<int64_t>
- Update all usages of get<int> to get<int64_t> for RequestId/ProgressToken
- Change pending_requests_ map key from int to int64_t in RequestTracker
- Cast uint64_t request ID counters to int64_t to avoid overflow
The compiler warned that member 'mode_' was being initialized before
'effective_level_' despite appearing later in the initializer list.
Reordered the initializer list to match the member declaration order
in the class (effective_level_, name_, mode_) to eliminate the warning.

This follows C++ best practice where initialization order follows
declaration order, not initializer list order.
The compiler warned about deleting a non-final class with virtual
functions through a base pointer without a virtual destructor.
Made the destructor virtual to ensure proper cleanup when
ConnectionStateMachine is used polymorphically.

This fixes the warning: 'delete called on non-final
ConnectionStateMachine that has virtual functions but
non-virtual destructor'
When using C++17 std::optional and std::variant, import the std
functions directly into mcp namespace using 'using' declarations
instead of creating wrapper template functions. This avoids
ambiguous function call errors between std:: and mcp:: versions.

Changes:
- Replace make_optional wrapper templates with 'using std::make_optional'
- Replace get/get_if/holds_alternative/visit wrapper templates with
  'using std::*' declarations
- Keeps wrapper implementations only for C++14 compatibility path

This eliminates ambiguity errors like:
  'call to make_optional is ambiguous'
  'call to holds_alternative is ambiguous'
  'call to get is ambiguous'
SystemError struct uses std::string but the header was missing.
This caused compilation errors about implicit instantiation of
undefined template 'std::basic_string<char>' when io_result.h
was included before any header that transitively included <string>.

Added explicit #include <string> to fix the dependency.
Added make_overload() factory function that creates an overload object
from a set of lambda functions. This provides a cleaner syntax for
std::visit with multiple visitor lambdas.

The helper forwards arguments to construct the overload pattern and
enables usage like:
  visit(make_overload(
    [](Type1) { ... },
    [](Type2) { ... }
  ), variant_obj);

This was being used in metrics_filter.h but was not defined,
causing compilation errors.
Changed stdio_pipe_transport.h to include mcp/core/compat.h instead
of mcp/core/optional.h directly. This ensures proper selection between
std::optional (C++17) and mcp::optional (C++14) based on the compiler
version.

Direct inclusion of optional.h caused conflicts when C++17 was enabled,
as both std::optional and mcp::optional definitions would be present.
The compat.h header provides the correct abstraction layer.
@github-actions
Copy link

github-actions bot commented Oct 5, 2025

❌ Code Formatting Check Failed

Some files in this PR are not properly formatted according to the project's clang-format rules.

To fix this issue:

make format

Then commit and push the changes.

@github-actions
Copy link

github-actions bot commented Oct 5, 2025

❌ Code Formatting Check Failed

Some files in this PR are not properly formatted according to the project's clang-format rules.

To fix this issue:

make format

Then commit and push the changes.

gophergogo added 3 commits October 6, 2025 09:54
Updated C API type conversion functions to handle int64_t instead of
int for numeric RequestId and ProgressToken values, matching the core
type definition changes.

Changes:
- mcp_c_type_conversions.h: Updated to_cpp_request_id, to_c_request_id,
  to_cpp_progress_token, and to_c_progress_token to use int64_t
- mcp_c_bridge.h: Updated to_c_request_id_safe and to_cpp_request_id_safe
  to use int64_t

This ensures C API builds correctly with the updated variant types and
prevents overflow issues with large request IDs.
Remove --verbose flag from ctest command to make test output less noisy.
Now only shows test progress and summary instead of full gtest output.
Add make format-cpp target to format only C++ files with clang-format,
separate from the all-languages format target.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Move MCP_SO_ATTACH_REUSEPORT_CBPF constant definition to top of file
before it's used in ReusePortSocketOption::setOptionForListen().
Previously it was defined after use, causing compilation error.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
The filesystem header is C++17 only and wasn't being used. Removed
unused includes to maintain C++14 compatibility for default builds.
gophergogo added 2 commits October 6, 2025 18:08
Move MCP_SO_ATTACH_REUSEPORT_CBPF constant definition to top of file
before it's used in ReusePortSocketOption::setOptionForListen().
Previously it was defined after use, causing compilation error.
….cc (#122)

The filesystem header is C++17 only and wasn't being used. Removed
unused includes to maintain C++14 compatibility for default builds.
gophergogo added 2 commits October 6, 2025 20:17
…ts for C++14 compatibility (#122)

Replace C++20-style designated initializers with regular field assignments
to support older compilers (GCC 6.x on Ubuntu 18). Designated initializers
with skipped fields are not supported in C++14/C++17.
…s for C++14 (#122)

Replace designated initializers with explicit field assignments in
listener implementations for C++14 compatibility.

Also fix reference_wrapper type mismatch in getAllListeners() by
explicitly casting ActiveListener to base Listener class.
gophergogo added 6 commits October 6, 2025 20:22
…listener_impl.cc (#122)

Replace designated initializer at line 539 with explicit field
assignments for C++14 compatibility.
)

Add missing <atomic> and <unordered_map> headers to ssl_context.h.

Replace C++17 structured bindings with traditional pair access in
ssl_state_machine.cc for C++14 compatibility.
…air access (#122)

Replace all structured bindings (C++17 feature) with traditional
.first/.second access for C++14 compatibility in:
- http_parser.cc
- mcp_client.cc
- log_formatter.cc
- logger_registry.cc
…ibrary (#122)

Add POSITION_INDEPENDENT_CODE property to llhttp library to enable
linking into shared libraries. Fixes linker error on Linux when building
shared libgopher-mcp.so.
…og_sink.cc (#122)

Replace C++17 <filesystem> with C functions for C++14 compatibility:
- fs::exists() -> stat() based file_exists()
- fs::remove() -> std::remove()
…library (#122)

Add POSITION_INDEPENDENT_CODE property to nghttp2_static library to
enable linking into shared libraries. Fixes linker error on Linux when
building shared libgopher-mcp.so.
@edfams edfams removed their request for review October 7, 2025 17:32
@gophergogo gophergogo requested a review from edfams October 7, 2025 17:32
@gophergogo gophergogo merged commit deef7ce into main Oct 7, 2025
1 of 2 checks passed
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
The compiler warned that member 'mode_' was being initialized before
'effective_level_' despite appearing later in the initializer list.
Reordered the initializer list to match the member declaration order
in the class (effective_level_, name_, mode_) to eliminate the warning.

This follows C++ best practice where initialization order follows
declaration order, not initializer list order.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
The compiler warned about deleting a non-final class with virtual
functions through a base pointer without a virtual destructor.
Made the destructor virtual to ensure proper cleanup when
ConnectionStateMachine is used polymorphically.

This fixes the warning: 'delete called on non-final
ConnectionStateMachine that has virtual functions but
non-virtual destructor'
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
SystemError struct uses std::string but the header was missing.
This caused compilation errors about implicit instantiation of
undefined template 'std::basic_string<char>' when io_result.h
was included before any header that transitively included <string>.

Added explicit #include <string> to fix the dependency.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Remove --verbose flag from ctest command to make test output less noisy.
Now only shows test progress and summary instead of full gtest output.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Add make format-cpp target to format only C++ files with clang-format,
separate from the all-languages format target.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Move MCP_SO_ATTACH_REUSEPORT_CBPF constant definition to top of file
before it's used in ReusePortSocketOption::setOptionForListen().
Previously it was defined after use, causing compilation error.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
….cc (#122)

The filesystem header is C++17 only and wasn't being used. Removed
unused includes to maintain C++14 compatibility for default builds.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…ts for C++14 compatibility (#122)

Replace C++20-style designated initializers with regular field assignments
to support older compilers (GCC 6.x on Ubuntu 18). Designated initializers
with skipped fields are not supported in C++14/C++17.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…s for C++14 (#122)

Replace designated initializers with explicit field assignments in
listener implementations for C++14 compatibility.

Also fix reference_wrapper type mismatch in getAllListeners() by
explicitly casting ActiveListener to base Listener class.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…listener_impl.cc (#122)

Replace designated initializer at line 539 with explicit field
assignments for C++14 compatibility.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
)

Add missing <atomic> and <unordered_map> headers to ssl_context.h.

Replace C++17 structured bindings with traditional pair access in
ssl_state_machine.cc for C++14 compatibility.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…air access (#122)

Replace all structured bindings (C++17 feature) with traditional
.first/.second access for C++14 compatibility in:
- http_parser.cc
- mcp_client.cc
- log_formatter.cc
- logger_registry.cc
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…ibrary (#122)

Add POSITION_INDEPENDENT_CODE property to llhttp library to enable
linking into shared libraries. Fixes linker error on Linux when building
shared libgopher-mcp.so.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…og_sink.cc (#122)

Replace C++17 <filesystem> with C functions for C++14 compatibility:
- fs::exists() -> stat() based file_exists()
- fs::remove() -> std::remove()
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…library (#122)

Add POSITION_INDEPENDENT_CODE property to nghttp2_static library to
enable linking into shared libraries. Fixes linker error on Linux when
building shared libgopher-mcp.so.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Remove constexpr from string_literal constructor and operator== for
C++14 builds since C++14 doesn't allow loops in constexpr functions.
Keep constexpr for C++17+ builds.

Update tests to use runtime checks instead of static_assert for C++14.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…oop.cc (#122)

Explicitly initialize std::atomic<time_point> in constructor initializer
list. Default construction of atomic complex types is not supported in
older compilers (GCC 7).
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
….cc (#122)

Replace SocketCreationOptions designated initializers with lambda
functions returning initialized structs for C++14 compatibility.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
GCC 7 doesn't allow __attribute__((nothrow)) on function definitions,
only on declarations. Remove it for GCC/Clang builds to fix C API
compilation errors.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Add <cstring> header for strncpy and memset functions used in C API
error handling.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
SO_NOSIGPIPE is macOS-specific and doesn't exist on Linux. Add
conditional compilation guard to only use it when available.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
Add <csignal> header for SIGINT and signal() functions used in the example.
This header is required for signal handling functionality on Ubuntu 18 with GCC 7.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…#122)

Add missing <algorithm> header for std::find and add explicit bool return
type to lambda to work around GCC 7 type deduction issue with condition_variable.
gophergogo pushed a commit that referenced this pull request Oct 7, 2025
…g test (#122)

Replace C++17 <filesystem> with <cstdio> and <sys/stat.h>, using stat() for
file_exists() and std::remove() for file deletion to support C++14 on GCC 7.
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.

3 participants