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

[DO NOT review/merge it - just proposal] Separated config reading code from the rest logic #157

Open
wants to merge 2 commits into
base: development/v.0.0.2
Choose a base branch
from

Conversation

cybay
Copy link
Contributor

@cybay cybay commented Nov 16, 2018

DISCLAIMER:
DO NOT Review it
DO NOT Merge it
Only for discussing.
Just proposal.

Statements/changes

  1. graft-ng code should be located inside of graft::supernode namespace. Just because it IS graft supernode.
  2. Object Server (and its parts, e.g. Config) is located in graft::supernode::server
  3. Entity supernode now is called graft::supernode::Node. Its config is graft::supernode::Config
  4. Each config has its own loader, e.g. graft::supernode::server::ConfigLoader and
    graft::supernode::ConfigLoader. It does load data from command-line + ini-file into appropriate structure.
  5. ConfigOpts - renamed into graft::supernode::server::Config
  6. ConfigOptsEx - renamed into graft::supernode::Config

Decomposition of RouterT

  1. All related parts now located at graft::supernode::route
  2. RouterT actually never been the thing that does make routing. It was just a container for routes. Hence now it's called RouteSet.
  3. RouterT is not templated class any more. It was never needed.
  4. RouterT::Root - this is really router. And now ti's called graft::supernode::route::Router.

class ConfigLoader
{
public:
ConfigLoader(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need (void)? Is C somewhere assumed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

void is not related to C or C++. What the problem?

Copy link
Contributor

@jagerman jagerman Nov 21, 2018

Choose a reason for hiding this comment

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

It is most definitely related to C and is only even allowed in C++ because it needs to be compatible with C code. (But since this is a constructor it is especially pointless since C compatibility isn't even possible in the first place). See the C++ Core Guideline's strong recommendation against this.

size_t stake_wallet_refresh_interval_ms;

std::string watchonly_wallets_path; // path to watch-only wallets (supernodes)
bool testnet; // testnet flag
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not "bool testnet = false;" or ""bool testnet = {}" instead of "Config(void);" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I'd prefer use One way of initialization instead of two different ways.
And we can't always initialize all things inplace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants