Skip to content

Optimize and add ability to configure error printing function #11

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Optimize and add ability to configure error printing function #11

Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a020752
Optimize and add ability to configure error printing function
rayhamel May 31, 2021
ba09c4b
Make parameters to print functions const, for consistency
rayhamel May 31, 2021
732627c
Use operator<< instead of ostream::write for string_view
rayhamel May 31, 2021
c369831
'invert' and eliminate repeated code in the stdio and iostream print_…
rayhamel May 31, 2021
e006adb
Switch back to ostream::write
rayhamel May 31, 2021
0d28933
Change error printing customization point to a variable (function poi…
rayhamel May 31, 2021
92914b2
Add tests for iostream output
rayhamel May 31, 2021
4264c17
Make clang-tidy happy with namespace closing comment
rayhamel Jun 1, 2021
09d4baf
Make print_error an inline variable to silence clang-tidy warning
rayhamel Jun 1, 2021
43b4f9a
Define a stub for cjdb::print_error even when CJDB_SKIP_STDIO is defi…
rayhamel Jun 1, 2021
6977cc1
Update documentation
rayhamel Jun 1, 2021
d553310
Minor formatting change
rayhamel Jun 1, 2021
e2bc28d
Another formatting change
rayhamel Jun 1, 2021
539b14e
Deleted asterisk, misc formatting
rayhamel Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions include/cjdb/contracts.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,42 @@
#ifndef CJDB_CONTRACTS_HPP
#define CJDB_CONTRACTS_HPP

#include <cstdio>
#include <string_view>
#include <type_traits>

#ifdef _MSC_VER
#define CJDB_PRETTY_FUNCTION __FUNCSIG__
#define CJDB_FORCE_INLINE __forceinline
#else
#define CJDB_PRETTY_FUNCTION __PRETTY_FUNCTION__
#define CJDB_FORCE_INLINE [[gnu::always_inline]] inline
#endif // _MSC_VER

#ifndef CJDB_PRINT_ERROR
Copy link
Owner

Choose a reason for hiding this comment

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

What is this guard protecting against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea is that the user could optionally define their own CJDB_PRINT_ERROR function.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rayhamel rayhamel May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rayhamel rayhamel Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#ifdef CJDB_USE_IOSTREAM
#include <iostream>
#else
#include <cstdio>
#endif // CJDB_USE_IOSTREAM

namespace cjdb::contracts_detail {
struct print_error_fn {
CJDB_FORCE_INLINE void operator()(std::string_view const message) const noexcept
#ifdef CJDB_USE_IOSTREAM
try {
std::clog.write(message.data(), static_cast<std::streamsize>(message.size()));
} catch(...) {}
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I got rid of that and for consistency made the stdio implementation throw when fwrite fails.

#else
{
std::fwrite(message.data(), sizeof(char), message.size(), stderr);
}
#endif // CJDB_USE_IOSTREAM
};
inline constexpr auto print_error = print_error_fn{};
} // namespace cjdb::contracts_detail
#define CJDB_PRINT_ERROR(MESSAGE) ::cjdb::contracts_detail::print_error(MESSAGE)
#endif // CJDB_PRINT_ERROR

// clang-tidy doesn't yet support this
//
// #ifndef __cpp_lib_is_constant_evaluated
Expand All @@ -18,12 +50,6 @@
#define CJDB_ASSERT(...) CJDB_CONTRACT_IMPL("assertion", __VA_ARGS__)
#define CJDB_ENSURES(...) CJDB_CONTRACT_IMPL("post-condition", __VA_ARGS__)

#ifdef _MSC_VER
#define CJDB_PRETTY_FUNCTION __FUNCSIG__
#else
#define CJDB_PRETTY_FUNCTION __PRETTY_FUNCTION__
#endif // _MSC_VER

namespace cjdb::contracts_detail {
#ifdef NDEBUG
inline constexpr auto is_debug = false;
Expand All @@ -39,7 +65,9 @@ namespace cjdb::contracts_detail {
if (not result) {
if (not std::is_constant_evaluated()) {
if constexpr (is_debug) {
std::fprintf(stderr, "%s in `%s`\n", message.data(), function.data());
CJDB_PRINT_ERROR(message);
CJDB_PRINT_ERROR(function);
CJDB_PRINT_ERROR("`\n");
}
}
#ifdef _MSC_VER
Expand Down Expand Up @@ -69,7 +97,7 @@ namespace cjdb::contracts_detail {

#define CJDB_CONTRACT_IMPL(CJDB_KIND, ...) \
::cjdb::contracts_detail::contract_impl(::cjdb::contracts_detail::matches_bool(__VA_ARGS__), \
__FILE__ ":" CJDB_TO_STRING(__LINE__) ": " CJDB_KIND " `" #__VA_ARGS__ "` failed", \
__FILE__ ":" CJDB_TO_STRING(__LINE__) ": " CJDB_KIND " `" #__VA_ARGS__ "` failed in `", \
CJDB_PRETTY_FUNCTION)


Expand Down