isQuorumSetSane: Add error string parameter #2725
Conversation
src/scp/BallotProtocol.cpp
Outdated
| if (errString != nullptr) | ||
| { | ||
| std::string msg(errString); | ||
| CLOG(DEBUG, "SCP") << msg; | ||
| } |
There was a problem hiding this comment.
Can someone suggest a way to inline this into the previous CLOG call?
There was a problem hiding this comment.
I would just write this as
CLOG(DEBUG, "SCP") << "Invalid quorum set received : " << (errString?errString:"<empty>"); and be done with it (also avoids a new variable).
AndrejMitrovic
force-pushed
the
sanity-reason
branch
from
dfa9be6
to
8a6e6e2
Compare
src/main/Config.cpp
Outdated
| "between {} and 100)", | ||
| UNSAFE_QUORUM ? 1 : 51); | ||
| std::string msg(errString); | ||
| LOG(FATAL) << "Invalid QUORUM_SET: " << msg; |
There was a problem hiding this comment.
why not just keep fmt::format("Invalid QUORUM_SET: {}", errString)
src/scp/BallotProtocol.cpp
Outdated
| if (errString != nullptr) | ||
| { | ||
| std::string msg(errString); | ||
| CLOG(DEBUG, "SCP") << msg; | ||
| } |
There was a problem hiding this comment.
I would just write this as
CLOG(DEBUG, "SCP") << "Invalid quorum set received : " << (errString?errString:"<empty>"); and be done with it (also avoids a new variable).
|
Also, before we can merge your changes, please fill in the agreement per https://github.com/stellar/.github/blob/master/CONTRIBUTING.md#submitting-changes |
AndrejMitrovic
force-pushed
the
sanity-reason
branch
from
8a6e6e2
to
8e2a988
Compare
|
Thanks! I've filled in the agreement and applied the changes. |
|
Thank you! |
|
r+ 8e2a98896b95de94acf99a6ab92fe9e878b08695 |
AndrejMitrovic
force-pushed
the
sanity-reason
branch
from
8e2a988
to
e44e88a
Compare
|
I've re-run clang formatter on the code. Sorry about that! |
|
r+ e44e88a11c22db126481d303911781db70bdd7d0 |
|
It looks like Travis uses clag-format 5, and I used the one installed with brew (v10) which has a slightly different output. I'll try to get v5 installed and re-run the formatter. |
If the quorum is considered invalid then the 'errString' parameter will contain the string explaning the reason why the quorum is considered invalid. Resolves stellar#2723
AndrejMitrovic
force-pushed
the
sanity-reason
branch
from
e44e88a
to
cbefaa9
Compare
|
Installing clang-format 5 seems to be tediously slow (tried https://gist.github.com/ffeu/6ffb75d8e8c7d92c0fbeb4b036599c33), so I applied to diff manually based on the results from Travis. |
|
@MonsieurNicolas : Since you got Travis set up, I wonder, why doesn't it trigger on PRs? Not having the CI trigger before approval (and only once per committer's approval) can make contributing quite tedious. |
|
@AndrejMitrovic yes this is the way to do it, the error message is an actual patch that can be applied if you don't have clang-format-5 installed. Let's see if it works this time around... |
|
@Geod24 good idea. I just enabled it for future PRs, from what I remember it was just too slow (we had it enabled a long time ago), but maybe it's not that bad now. |
|
r+ cbefaa9 |
Please feel free to suggest improvements. Thanks!
Resolves #2723