Skip to content
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.

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

Error type and config handling #1416

Merged
merged 12 commits into from Jan 14, 2019

Conversation

Projects
4 participants
@cryptocode
Copy link
Collaborator

commented Nov 30, 2018

New error type

As mentioned in issue #1111, users should get useful information when there's a config error. This PR introduces a nano::error type that wraps a std::error_code along with a custom error string.

It's able to consume bool error flags, std::error_code, boost::system::error_code and std::exception (all of which are used in the code base). It should be a good alternative in cases where dynamic error messages are required or when multiple types of errors must be handled (exceptions, bools, boost ec etc).

The nano::error type has some documentation here (I'll expand it) https://github.com/cryptocode/notes/wiki/Error-handling

std::error_code should still be used in most situations, and nano::error is able to convert to and from it.

JSON config abstraction

A nano::jsonconfig wrapper around boost's ptree is added. It uses nano::error to communicate error messages, which are for the most part automatically constructed from key name and type information. So if you're reading a port number of type uint16_t and the config contains an invalid value, you'll get "Error deserializing config: port is not an integer between 0 and 65535"

In addition to automated messages, range checks and so on are given a descriptive error, such as "At least one representative account must be set"

nano::jsonconfig should make it easier to eventually move the config to a proper JSON parser.

EDIT: pr is ready for review

@cryptocode cryptocode self-assigned this Nov 30, 2018

@cryptocode cryptocode removed the incomplete label Dec 1, 2018

@rkeene rkeene added this to the V18.0 milestone Dec 8, 2018

@rkeene rkeene added the enhancement label Dec 8, 2018

@cryptocode cryptocode force-pushed the cryptocode:config-error-handling branch 2 times, most recently from 90d4b75 to 1f82fb9 Dec 21, 2018

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 22, 2018

@wezrule seems like some commit comments got removed when I amended by mistake.

What I was saying was that constexpr will (on gcc) produce

enclosing class of constexpr non-static member function is not a literal type constexpr int json_version () const, i.e. constructor is not constexpr.

So inline is what seems to work across the board. C++ inline variables will be perfect for this I think.

@wezrule

This comment has been minimized.

Copy link
Collaborator

commented Dec 23, 2018

@cryptocode No worries. For anyone else reading, I originally commented on 1f82fb9 querying why constexpr was removed instead of inline (as inline is implicit with constexpr) as I thought that's what the warning was about originally, but it was a misguided assumption.

I tried this in compiler explorer (with c++14 compiler option):

class C {
  C();
  constexpr int f() { return 0; }
};

It only appears legal with versions gcc 7.2/clang 3.6 and greater. Curious, I did some more research and it looks like it was an oversight in the original spec to not allow constexpr static/non-static member functions of non-literal classes which has since been rectified:
https://groups.google.com/a/isocpp.org/forum/embed/#!topic/std-discussion/6jM8M8FUs30

So for greater C++14 compiler support not using them is beneficial (more reason to get #1044 done 👍). Although I'm not entirely sure why it was changed from a static constexpr variable to begin with, was this related to compiler support as well? I don't want to dwell too much on these minor details (there's plenty of better things to be doing!), but I will just add that member functions defined inside a class body are already implicitly inlined, so using the inline specifier is redundant.

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 23, 2018

@wezrule IIRC static constexpr ran into ODR-used issue in one instance (on some compiler) - something that's apparently fixed in C++17.

Removed the superfluous inline specifier (I have other PRs with it as well; old habit. I'll update them later)

@zhyatt zhyatt added this to Unscheduled in V18 Dec 27, 2018

@cryptocode cryptocode referenced this pull request Dec 28, 2018

Closed

Config error handling #1111

@cryptocode cryptocode moved this from Unscheduled to CP 3 (2018-01-23) in V18 Dec 28, 2018

@cryptocode cryptocode moved this from CP 3 (2018-01-23) to CP 2 (2018-01-16) in V18 Dec 28, 2018

@cryptocode cryptocode referenced this pull request Dec 29, 2018

Merged

Support external RPC servers via IPC #1434

7 of 7 tasks complete

@rkeene rkeene force-pushed the cryptocode:config-error-handling branch from 1e071cf to 4ba120a Dec 30, 2018

@cryptocode cryptocode force-pushed the cryptocode:config-error-handling branch from 4ba120a to 69a2c41 Jan 3, 2019

@zhyatt zhyatt requested a review from wezrule Jan 7, 2019

@wezrule
Copy link
Collaborator

left a comment

Tested a few different errors in config.json and works fab!
I guess the next step is to have a gui for this to stop hand-written problems altogether!

Show resolved Hide resolved nano/lib/errors.hpp
Show resolved Hide resolved nano/nano_node/daemon.hpp
Show resolved Hide resolved nano/lib/errors.hpp Outdated
Show resolved Hide resolved nano/lib/jsonconfig.hpp Outdated
Show resolved Hide resolved nano/lib/jsonconfig.hpp Outdated
Show resolved Hide resolved nano/lib/jsonconfig.hpp Outdated
Show resolved Hide resolved nano/lib/jsonconfig.hpp Outdated
Show resolved Hide resolved nano/nano_node/daemon.hpp
Show resolved Hide resolved nano/core_test/uint256_union.cpp Outdated
Show resolved Hide resolved nano/node/nodeconfig.cpp

@cryptocode cryptocode force-pushed the cryptocode:config-error-handling branch from 69a2c41 to 4bbaf59 Jan 9, 2019

cryptocode added some commits Jan 9, 2019

cryptocode added some commits Jan 10, 2019

Show resolved Hide resolved nano/lib/errors.hpp Outdated
Show resolved Hide resolved nano/nano_node/daemon.hpp
Show resolved Hide resolved nano/lib/jsonconfig.hpp
Show resolved Hide resolved nano/core_test/uint256_union.cpp Outdated
Show resolved Hide resolved nano/lib/errors.hpp Outdated
@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 10, 2019

@wezrule

So I think you can remove all the user defined constructors because they will be auto generated for you doing the same. If not then they can at least be = default.

Removing didn't compile on clang, but they can probably be defaulted. I'll look into it again.

@cryptocode cryptocode merged commit 282cc0d into nanocurrency:master Jan 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.