Modifications and cleanup for compilation on OS/X#42
Conversation
Added various include directives in http_response.cpp and http_resource.cpp and modified http_endpoint copy constructor to make it public to satifify clang. Fixed typo/bug in http_request::set_requestor_port. Modified various forwards to match actual declarations. Modified configure to allow build in same directory.
|
Thank you for your help. I discuss here the changes. I am a lot interested in having details especially regarding the points 1 and 3.
|
|
|
Don't worry, I will probably anyway move the pull request manually. I installed a clang instance to test it and see if I can obtain something that actually works fine with both compilers. Regarding 7) yes, I really don't like compilations in the same folder - they usually end up in mixups between code and binaries that more than once have made me cry blood to be solved. Once I move the pull request (or part of it) inside the code, I will add you in the AUTHORS file as a collaborator in the format: FirstName FamilyName emailAddress let me know if for any reason you don't want to have your name or your e-mail address inserted there. |
|
Cool! Name: Jeff Waller Also, just to make it fun, XCode 5 clang is not the latest clang... clang --version
In file included from webserver.cpp:23: ... /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:1505:36: error: calling a private constructor of class 'httpserver::details::http_endpoint' |
|
I made some changes. It seems to compile on both my clang 3.2 installation and clang 3.3 on travis-ci. Let me know if the current trunk works on your clang 5.0. |
|
Hey I like the configure option! Unfortunately, I think OS/X std impl is going to insist that http_endpoint is publicly available it's essentially the same error as before. Which appears to arise from this one statement: Makes sense too because of this: webserver.hpp:232 if instead it was something like or this second option is probably going to force std=c++11 though, which is limiting, but maybe no big deal in 2014. that would probably satisfy the OS/X impl. mv -f .deps/string_utilities.Tpo .deps/string_utilities.Plo |
|
I think they are avoiding somehow the type erasure in new versions - since it compiles with 3.2 and 3.3. |
|
Oops, haha, you're right of course, but then same comments, but for the key not the value. |
|
I tried to block the possibilities for clang to build the partial object. I do not see why clang need to build a temporary copy of the key while inserting - this actually does not happen in linux implementation of LLVM 3.3 that matches your version on Mac OS/X. This make me strongly suspect that Mac implementation is just wrong. It would be long to explain the details of why I need an object and not a pointer as key and it is much more easy to explain why it is not possible to use C++11 costructs since there are guys compiling this library with gcc 4.1. I still think that having a public constructor for http_endpoint is not a good choice and I will not change this because they are not able to build a decent compiler. Said this, let me know if this new version works, otherwise I will add an #ifdef that moves to public the constructor in OS/X environments - at least this shame will be confined. |
Address all 48 minor findings from the 2026-05-26 review pass. Summary of changes by file: src/detail/webserver_aliases.cpp - Remove redundant `static` from `append_sanitized` (anon namespace already provides internal linkage; findings #6/#15/#16) - Trim verbose format-compatibility comment to single-line reference (#7) src/detail/webserver_finalize.cpp - Trim file-level comment block to two-sentence summary (#20) - Add clarifying comment to the `!mr->response` defensive guard (#21) - Rename `bytes` → `bytes_queued` to match the ctx field it populates (#22) - Add inline NOTE to elapsed ternary: alias must not read ctx.elapsed (#36) - Sentinel check for degenerate start_time: emit nanoseconds{-1} when answer_to_connection never ran, so hook authors can distinguish port-scan paths from real (but very slow) requests (#37) src/detail/webserver_request.cpp - Remove redundant `mr->ws = parent` from complete_request; add comment noting the field is pre-populated in answer_to_connection (#3) - Add NOTE comments at both before_handler and skip_handler short-circuit paths documenting that after_handler does not fire there (#38) - Collapse stale TASK-050 migration comment to one line (#25) src/httpserver/hook_context.hpp - Add @note to response_sent_ctx documenting elapsed==zero when only the log_access alias fires (no add_hook(response_sent, ...) registered) (#9) - Add @note to request_completed_ctx documenting the nanoseconds{-1} sentinel for degenerate start_time paths (#37) src/httpserver/create_webserver.hpp - Add @param note to log_access() setter documenting that the callable must be CopyConstructible (#35) examples/clf_access_log.cpp - Refactor emit_clf_line to use early null guard + unconditional extraction of method/path (idiomatic pattern matching webserver_aliases.cpp) (#12-14) - Add comment explaining intentional 'HTTP/1.1' hardcoding (#33) specs/architecture/04-components/hooks.md - Update after_handler, response_sent, request_completed rows with file:symbol fire-site references (#26, #27) - Fix stale webserver.cpp references for route_resolved, before_handler, and handler_exception rows (pre-existing staleness from TASK-048) (#27) - Update API surface comment: after_handler_ctx uses http_response* not http_response& (#4) - Add after_handler firing rules paragraph documenting which paths fire / skip after_handler (#1, #2) specs/tasks/M5-routing-lifecycle/TASK-050.md - Update three action items to reference correct TUs (webserver_finalize.cpp, webserver_callbacks.cpp) and correct field types (#28, #40) test/integ/hooks_no_firing.cpp - Add positive firing-count assertions for all wired phases on the happy-path GET (after_handler, response_sent, request_completed, route_resolved, before_handler, connection_opened, connection_closed, request_received) to give the test lasting regression value (#10, #42) test/integ/hooks_request_completed_fires_on_early_failure.cpp - Remove timing-dependent 50ms sleep; rely on ws.stop() as synchronisation barrier, consistent with other integ tests in this task (#11, #43) test/unit/hooks_log_access_alias_slot_test.cpp - Add assertion that '-' replacement appears at injection site in path sanitization test (not just absence of control chars) (#44) - Add assertion that 'GET' remains intact in method sanitization test (#46) - Add fourth test case pinning construction-time isolation between two webservers each with their own log_access callable; documents that runtime re-registration is deferred to a future task (#48) specs/unworked_review_issues/2026-05-26_123948_task-050.md - Mark all 48 items with [x] and disposition notes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eview-cleanup) Major findings (5 total): - #1 (adr-violation): implementation is correct per DR-012/DR-009 §5.2; deferred. - #2/#3/#28 (code-structure, triplicate): extracted append_impl<P,Sig> template helper in resource_hook_table.cpp anonymous namespace; each of the five append_* methods now delegates in one line (mirrors fire_short_circuit_impl / fire_void_impl pattern). - #4/#5 (test-structure, advisory): deferred — project prefers per-case explicit test bodies for independent failure reporting. Key minor fixes applied (cosmetic, no behavior change): - TOCTOU anti-pattern (#6/#35/#36/#45/#47/#48): removed expired()+lock() double-check from per_route_table() helper; fire_request_completed_gated now uses the helper consistently (was inline-expanded). - Shadow variable (#15/#38): renamed local var per_route_table → rtable in fire_before_handler_gated, consistent with other gated-fire helpers. - Lifetime comment (#12): added "res keeps the resource alive while rtable is in use" note in handle_dispatch_exception. - Memory-order comment (#50): documented acquire-chain at rtable fetch site in fire_before_handler_gated. - Sentinel assertions (#41/#61/#62): removed LT_CHECK_EQ(true, true) from hooks_per_route_resource_destroyed_first.cpp and hook_api_shape_test.cpp; replaced with descriptive comments. - resource_hook_table.hpp comments (#8): clarified named-vector vs std::array tradeoff and any_hooks_ unused slots. - http_resource.hpp (#24/#27): added copy-shares-hook-table note; added comment before HTTPSERVER_COMPILATION guard. - http-resource.md / DR-012.md (#9/#42/#43): documented per-route hook bus and PIMPL storage choice. All 62 items marked [x] in specs/unworked_review_issues/2026-05-26_230100_task-051.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
I've made modifications mostly for compilation on OS/X to succeed. Found 1 generic bug and one bug that is OS/X specific. The resulting library have been tested by inclusion in a program compiled for both OS/X and Linux. The changes are limited in scope and can be verified readily by inspection. I also made some changes that are not strictly necessary, but suited to my development environment.
More specifically;
clang requires that the copy constructor of http_endpoint be public.
Changed to match declarations and forward declarations; class for class, struct for struct
clang may have further requirements needing fully defined types or there is a scenario not seen before that can give rise to use of a class not fully defined for which include file inclusion fixes the problem
OS/X AF_INET STREAM_SOCKETS do not support SOCK_CLOSEXEC; the system call returns EPROTONOSUPPORT not EINVAL.
Changed a getter to const as it doesn't change the class state and expands the scenarios in which a temporary object need not be created.
Fixed a typo in set_requestor_port.
Changed to allow building in the same directory, modified .gitignore to ignore the resulting targets