Optimize and add ability to configure error printing function #11
Conversation
include/cjdb/contracts.hpp
Outdated
| #define CJDB_FORCE_INLINE [[gnu::always_inline]] inline | ||
| #endif // _MSC_VER | ||
|
|
||
| #ifndef CJDB_PRINT_ERROR |
There was a problem hiding this comment.
What is this guard protecting against?
There was a problem hiding this comment.
My idea is that the user could optionally define their own CJDB_PRINT_ERROR function.
There was a problem hiding this comment.
cjdb::contracts_detail is a detail namespace, so a user can't do this under the proposed model. If you want to make this configurable, please put it in namespace cjdb. I think the macro should probably be opt-out, not opt-in? Otherwise folks won't get any output in their debug builds, which would be surprising.
There was a problem hiding this comment.
My idea is that the user could define or choose an appropriate function under whichever namespace, then define the function-like macro CJDB_PRINT_ERROR to call that function. The cjdb::contracts_detail::print_error function is only created when CJDB_PRINT_ERROR is not already defined.
There may of course be better ways of making this configurable.
There was a problem hiding this comment.
Could you please have a think about a more C++-oriented way to configure this? You don't need to implement it just yet: come back here and post the idea, but if we can avoid a macro, that'd be great.
There was a problem hiding this comment.
I changed the implementation to use a function pointer cjdb::print_error which the user can set to something else. I still do have two macro configuration options CJDB_USE_IOSTREAM (as you know, #include <iostream> introduces some overhead, so should be opt-in) and CJDB_SKIP_STDIO, in which case there's also no dependency on cstdio and the user must provide their own definition for cjdb::print_error.
include/cjdb/contracts.hpp
Outdated
| #define CJDB_FORCE_INLINE [[gnu::always_inline]] inline | ||
| #endif // _MSC_VER | ||
|
|
||
| #ifndef CJDB_PRINT_ERROR |
There was a problem hiding this comment.
cjdb::contracts_detail is a detail namespace, so a user can't do this under the proposed model. If you want to make this configurable, please put it in namespace cjdb. I think the macro should probably be opt-out, not opt-in? Otherwise folks won't get any output in their debug builds, which would be surprising.
include/cjdb/contracts.hpp
Outdated
| #define CJDB_FORCE_INLINE [[gnu::always_inline]] inline | ||
| #endif // _MSC_VER | ||
|
|
||
| #ifndef CJDB_PRINT_ERROR |
There was a problem hiding this comment.
Could you please have a think about a more C++-oriented way to configure this? You don't need to implement it just yet: come back here and post the idea, but if we can avoid a macro, that'd be great.
include/cjdb/contracts.hpp
Outdated
| try { | ||
| std::clog.write(message.data(), static_cast<std::streamsize>(message.size())); | ||
| } catch(...) {} |
There was a problem hiding this comment.
try/catch block isn't necessary: if the stream throws in the middle of reporting a contract violation, I think it would be more useful if the user discovered that.
There was a problem hiding this comment.
Ok, I got rid of that and for consistency made the stdio implementation throw when fwrite fails.
…nter) instead of a macro
I added tests and a section to the documentation. Thanks for the feedback and your time. |
include/cjdb/contracts.hpp
Outdated
| inline print_error_fn* print_error = [](std::string_view message) { | ||
| #ifdef CJDB_USE_IOSTREAM | ||
| std::cerr.write(message.data(), static_cast<std::streamsize>(message.size())); | ||
| #elif !defined(CJDB_SKIP_STDIO) |
There was a problem hiding this comment.
What is the motivation for skipping I/O?
There was a problem hiding this comment.
- For use in freestanding implementations that lack an output device and/or the header
cstdio - For users who provide their own
cjdb::print_errorfunction. While this use case is possible without a configuration option, it's not necessarily desirable to#include <cstdio>with all that implies (e.g. declaring a function calledremovein the global namespace).
|
Thanks for your patience @rayhamel. I wanted a bit of time to think over your comments, but I've been very preoccupied recently. I'll get to this as soon as I can. |
Absolutely no rush! Thanks again for your time. |
|
Hi @rayhamel, to jog my memory on this PR (and the next one), your goal here is to add |
As mentioned in the original PR #9 , fixes a potential issue when the user has called
std::ios_base::sync_with_stdio(false)