Skip to content

clang-format -i (All Dash Related Files)#2348

Closed
Duality-CDOO wants to merge 2 commits into
dashpay:developfrom
Duality-CDOO:clang-format
Closed

clang-format -i (All Dash Related Files)#2348
Duality-CDOO wants to merge 2 commits into
dashpay:developfrom
Duality-CDOO:clang-format

Conversation

@Duality-CDOO
Copy link
Copy Markdown

This applies clang-format -i to all Dash specific files.

Includes for std::string and std::vector had to be added to masternodeconfig.h to allow successful build.

@nmarley
Copy link
Copy Markdown

nmarley commented Oct 13, 2018

If we're going to add this, we should probably (eventually?) add some kind of CI check for formatting also, otherwise eventually we'll get out of skew with clang-format rules again and have to do other formatting PRs in the future.

Copy link
Copy Markdown

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

@nmarley Not everything clang-format does makes it better imo, there are some weird things sometimes and there are things that make readability worse (and not applying them would be a false positive I guess and would break a CI build), see inline comments.

@Duality-CDOO this ^^^ + it's interesting to see the need for 20f8468 and that win32/64 builds are failing on your branch after (what initially looked like) a trivial refactoring. Looks like rearranging #includes revealed a couple of hidden issues (the later one can be fixed by UdjinM6@a7313d3 btw) but I would probably move code changes which fix them into a separate PR and keep this PR clang-format focused.

Comment thread src/activemasternode.cpp
mnp.fSentinelIsCurrent =
(abs(GetAdjustedTime() - nSentinelPingTime) < MASTERNODE_SENTINEL_PING_MAX_SECONDS);
if(!mnp.Sign(activeMasternodeInfo.keyOperator, activeMasternodeInfo.keyIDOperator)) {
(abs(GetAdjustedTime() - nSentinelPingTime) < MASTERNODE_SENTINEL_PING_MAX_SECONDS);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure why it's on a new line at all, I would just make it one-liner instead

Comment thread src/activemasternode.h
static const int ACTIVE_MASTERNODE_SYNC_IN_PROCESS = 1;
static const int ACTIVE_MASTERNODE_INPUT_TOO_NEW = 2;
static const int ACTIVE_MASTERNODE_NOT_CAPABLE = 3;
static const int ACTIVE_MASTERNODE_STARTED = 4;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd keep it as is. Same for all the other static consts in other header files.

Comment thread src/activemasternode.h
enum masternode_type_enum_t {
MASTERNODE_UNKNOWN = 0,
MASTERNODE_REMOTE = 1
MASTERNODE_REMOTE = 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd keep it as is. Same for all the other enums.

} std::cout
<< "CGovernanceTriggerManager::CleanAndRemove: Removing object: "
<< strDataAsPlainString
<< std::endl;);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's some clang-format weirdness we already had in some previous attempt, should be reverted.

// Let's see why this failed
for (const auto& payee : mnBlockPayees.second.vecPayees) {
for (const auto& payee
: mnBlockPayees.second.vecPayees) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

another weirdness, should be reverted

Comment thread src/rpc/masternode.cpp
(int64_t)(mn.lastPing.sigTime - mn.sigTime) << " " <<
mn.GetLastPaidTime() << " " <<
mn.GetLastPaidBlock();
streamInfo << mn.addr.ToString() << " " << CBitcoinAddress(mn.pubKeyCollateralAddress.GetID()).ToString() << " " << mn.GetStatus() << " " << mn.nProtocolVersion << " " << mn.lastPing.nDaemonVersion << " " << mn.lastPing.GetSentinelString() << " " << (mn.lastPing.fSentinelIsCurrent ? "current" : "expired") << " " << (int64_t)mn.lastPing.sigTime << " " << (int64_t)(mn.lastPing.sigTime - mn.sigTime) << " " << mn.GetLastPaidTime() << " " << mn.GetLastPaidBlock();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same

Comment thread src/rpc/masternode.cpp
+ HelpExampleRpc("sentinelping", "1.0.2")
);
"\nExamples:\n" +
HelpExampleCli("sentinelping", "1.0.2") + HelpExampleRpc("sentinelping", "1.0.2"));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd keep it as is

Comment thread src/masternode.h
masternode_info_t(int activeState, int protoVer, int64_t sTime,
COutPoint const& outpnt, CService const& addr,
CPubKey const& pkCollAddr, CPubKey const& pkMN) :
masternode_info_t(int activeState, int protoVer, int64_t sTime, COutPoint const& outpnt, CService const& addr, CPubKey const& pkCollAddr, CPubKey const& pkMN) :
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd keep it as is

Comment thread src/masternode.h
masternode_info_t(int activeState, int protoVer, int64_t sTime,
COutPoint const& outpnt, CService const& addr,
CKeyID const& pkCollAddr, CKeyID const& pkOwner, CKeyID const& pkOperator, CKeyID const& pkVoting) :
masternode_info_t(int activeState, int protoVer, int64_t sTime, COutPoint const& outpnt, CService const& addr, CKeyID const& pkCollAddr, CKeyID const& pkOwner, CKeyID const& pkOperator, CKeyID const& pkVoting) :
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same

Comment thread src/rpc/governance.cpp
vote_outcome_enum_t eVoteOutcome)
const uint256& hash,
vote_signal_enum_t eVoteSignal,
vote_outcome_enum_t eVoteOutcome)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd keep it as is

@Duality-CDOO
Copy link
Copy Markdown
Author

What this has showed us is that the includes for all files need looking at.

@UdjinM6
Copy link
Copy Markdown

UdjinM6 commented Nov 6, 2018

This one is superseded by a bunch of recent PRs by @PastaPastaPasta, closing.

@UdjinM6 UdjinM6 closed this Nov 6, 2018
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.

3 participants