Optimize and add ability to configure error printing function by rayhamel · Pull Request #11 · cjdb/constexpr-contracts

rayhamel · 2021-05-31T01:21:33Z

As mentioned in the original PR #9 , fixes a potential issue when the user has called std::ios_base::sync_with_stdio(false)

cjdb

Thanks for splitting this into multiple commits. Before approving, I'd like to see a few tests and the documentation updated, to demonstrate the new usage.

include/cjdb/contracts.hpp

cjdb · 2021-05-31T02:13:23Z

include/cjdb/contracts.hpp

+	#define CJDB_FORCE_INLINE [[gnu::always_inline]] inline
+#endif // _MSC_VER
+
+#ifndef CJDB_PRINT_ERROR


What is this guard protecting against?

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

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.

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.

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.

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

cjdb · 2021-05-31T03:18:30Z

include/cjdb/contracts.hpp

+	#define CJDB_FORCE_INLINE [[gnu::always_inline]] inline
+#endif // _MSC_VER
+
+#ifndef CJDB_PRINT_ERROR


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

…error implementations

cjdb

A few more minor comments.

Test cases and documentation will need to be updated in order for this patch to be approved.

cjdb · 2021-05-31T16:23:41Z

include/cjdb/contracts.hpp

+	#define CJDB_FORCE_INLINE [[gnu::always_inline]] inline
+#endif // _MSC_VER
+
+#ifndef CJDB_PRINT_ERROR


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.

cjdb · 2021-05-31T16:26:18Z

include/cjdb/contracts.hpp

+			try {
+				std::clog.write(message.data(), static_cast<std::streamsize>(message.size()));
+			} catch(...) {}


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.

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

include/cjdb/contracts.hpp

…nter) instead of a macro

…ned, to avoid UB

rayhamel · 2021-06-01T00:50:04Z

Thanks for splitting this into multiple commits. Before approving, I'd like to see a few tests and the documentation updated, to demonstrate the new usage.

I added tests and a section to the documentation. Thanks for the feedback and your time.

README.md

cjdb · 2021-06-01T03:51:10Z

include/cjdb/contracts.hpp

+	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)


What is the motivation for skipping I/O?

For use in freestanding implementations that lack an output device and/or the header cstdio

For users who provide their own cjdb::print_error function. 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 called remove in the global namespace).

cjdb · 2021-06-06T19:09:23Z

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.

rayhamel · 2021-06-07T15:23:54Z

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.

cjdb · 2022-02-18T20:10:56Z

Hi @rayhamel, to jog my memory on this PR (and the next one), your goal here is to add ostream support, right?

Optimize and add ability to configure error printing function

a020752

rayhamel mentioned this pull request May 31, 2021

Further optimized error printing #12

Open

rayhamel added 2 commits May 30, 2021 21:27

Make parameters to print functions const, for consistency

ba09c4b

Use operator<< instead of ostream::write for string_view

732627c

cjdb requested changes May 31, 2021

View reviewed changes

cjdb reviewed May 31, 2021

View reviewed changes

rayhamel added 2 commits May 30, 2021 23:44

'invert' and eliminate repeated code in the stdio and iostream print_…

c369831

…error implementations

Switch back to ostream::write

e006adb

cjdb requested changes May 31, 2021

View reviewed changes

rayhamel added 6 commits May 31, 2021 19:33

Change error printing customization point to a variable (function poi…

0d28933

…nter) instead of a macro

Add tests for iostream output

92914b2

Make clang-tidy happy with namespace closing comment

4264c17

Make print_error an inline variable to silence clang-tidy warning

09d4baf

Define a stub for cjdb::print_error even when CJDB_SKIP_STDIO is defi…

43b4f9a

…ned, to avoid UB

Update documentation

6977cc1

rayhamel added 2 commits May 31, 2021 20:51

Minor formatting change

d553310

Another formatting change

e2bc28d

rayhamel requested a review from cjdb June 1, 2021 02:05

cjdb reviewed Jun 1, 2021

View reviewed changes

Deleted asterisk, misc formatting

539b14e

Conversation

rayhamel commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjdb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rayhamel May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rayhamel Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjdb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rayhamel commented Jun 1, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjdb commented Jun 6, 2021

Uh oh!

rayhamel commented Jun 7, 2021

Uh oh!

cjdb commented Feb 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rayhamel commented May 31, 2021 •

edited

Loading

rayhamel May 31, 2021 •

edited

Loading

rayhamel Jun 1, 2021 •

edited

Loading