Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add syslog logger support. #386

Merged
merged 2 commits into from
Dec 5, 2014
Merged

Add syslog logger support. #386

merged 2 commits into from
Dec 5, 2014

Conversation

thughes
Copy link
Contributor

@thughes thughes commented Nov 26, 2014

Adding syslog as a supported logger. This seems like a common enough use-case to be in the main repo.

@thughes
Copy link
Contributor Author

thughes commented Nov 26, 2014

I don't think the build failure is related to my change (when running scons test locally all the tests pass).

@zaphoyd
Copy link
Owner

zaphoyd commented Nov 26, 2014

yep, the failure is in one of the timed tests. Travis machines are not always super fast so sometimes there are false positives on those.

@zaphoyd
Copy link
Owner

zaphoyd commented Nov 27, 2014

I'm not ready to introduce virtual methods to the logging interface. If you resubmit this without the virtual methods I'll accept.

@thughes
Copy link
Contributor Author

thughes commented Dec 1, 2014

@zaphoyd, I can definitely remove the virtual methods, but just to satisfy my curiosity, can I ask why? (e.g., worries about performance). The reason I marked it virtual (and used C++11's override) in the derived class was so that we would get a compiler error if you changed the signature of the method in the base class.

@zaphoyd
Copy link
Owner

zaphoyd commented Dec 1, 2014

Sure. A few reasons. Primarily, appropriate tools for the job and not bringing in additional runtime complexity unless it is necessary. WebSocket++'s policy based composition of components (including the logging system) is a compile time operation with no need for runtime polymorphism, as such virtual methods add complexity and overhead that is not needed. Keeping static binding is faster and gentler on caches/branch predictors, allows IDEs and the compiler to provide better error messages, better documentation links, better inlining, and other optimization.

This is particularly important for the logging system which gets invoked in many of the library's critical paths and in many cases those invocations are intended to be inlined into no-ops/branches that always fail, something which virtual dispatch complicates.

With respect to override, the library is required to be useable on non-C++11 systems, so the addition of new C++11 keywords needs to be done carefully. In practice, even without virtual+override, you will still get a compile error if the signature of the write methods change as the call sites in the library itself will change. The policy based design methods will enforce at compile time that all custom policy components implement all required method signatures.

WebSocket++ is designed to be extended via custom policies for certain components (logging, RNG, transport, http parsing). I consider the interfaces that the library uses to talk to these components a part of the public API and subject to the same backwards compatibility and breaking change notifications as the rest of the library. An update to the interface between the library core and policy components would be considered a major API breaking change that would not be made lightly and if done would be be listed prominently in the release notes.

@thughes
Copy link
Contributor Author

thughes commented Dec 2, 2014

Thanks, that's exactly what I was looking for. Glad to know that changing the logger interface would be considered a major API break.

@ido
Copy link

ido commented Dec 2, 2014

@zaphoyd Excellent explanation. 👍

@thughes
Copy link
Contributor Author

thughes commented Dec 5, 2014

@zaphoyd Removed the virtual methods and override.

@zaphoyd zaphoyd added the Feature label Dec 5, 2014
zaphoyd added a commit that referenced this pull request Dec 5, 2014
@zaphoyd zaphoyd merged commit 8a9b0be into zaphoyd:develop Dec 5, 2014
@zaphoyd zaphoyd added this to the 0.5.0 milestone Dec 5, 2014
@jrabek jrabek deleted the syslog branch March 12, 2015 22:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants