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

Remove TODO in DisputedTx.h about renaming constants for ConsensusParms #2614

Closed
wants to merge 2 commits into from

Conversation

JoeLoser
Copy link
Contributor

Based on a TODO comment in DisputedTX.h, it seems at one point the
data members of ConsensusParms were macros. Now that they are not,
we should spell them like other data members (without all uppercase).

Reviewers:
@wilsonianb @scottschurr

Based on a TODO comment in DisputedTX.h, it seems at one point the
data members of ConsensusParms were macros. Now that they are not,
we should spell them like other data members (without all uppercase).
@ripplelabs-jenkins
Copy link
Collaborator

Thank you for your submission. It will be reviewed soon and submitted for processing in CI.

@ripplelabs-jenkins
Copy link
Collaborator

ripplelabs-jenkins commented Jun 29, 2018

Jenkins Build Summary

Built from this commit

Built at 20180706 - 01:44:51

Test Results

Build Type Log Result Status
msvc.debug logfile 1032 cases, 0 failed, t: 568s PASS ✅
coverage logfile 1035 cases, 0 failed, t: 690s PASS ✅
docs logfile 1 cases, 0 failed, t: 2s PASS ✅
gcc.release.unity
-Dassert=true,
MANUAL_TESTS=true
logfile 1101 cases, 0 failed, t: 801s PASS ✅
clang.debug.unity logfile 1035 cases, 0 failed, t: 251s PASS ✅
clang.debug.nounity logfile 1033 cases, 0 failed, t: 288s PASS ✅
clang.debug.unity,
PARALLEL_TESTS=false
logfile 1035 cases, 0 failed, t: 492s PASS ✅
gcc.debug.nounity logfile 1033 cases, 0 failed, t: 129s PASS ✅
gcc.debug.unity logfile 1035 cases, 0 failed, t: 248s PASS ✅
clang.release.unity
-Dassert=true
logfile 1034 cases, 0 failed, t: 339s PASS ✅
msvc.debug,
PROJECT_NAME=rippled_classic
logfile 1032 cases, 0 failed, t: 1236s PASS ✅
gcc.release.unity
-Dassert=true
logfile 1034 cases, 0 failed, t: 355s PASS ✅
gcc.debug.unity
-Dstatic=true
logfile 1035 cases, 0 failed, t: 248s PASS ✅
rpm logfile 1034 cases, 0 failed, t: n/a PASS ✅
msvc.release logfile 1031 cases, 0 failed, t: 516s PASS ✅

@ripplelabs-jenkins
Copy link
Collaborator

Thank you for your submission. It will be reviewed soon and submitted for processing in CI.

@wilsonianb
Copy link
Contributor

I'm leaning towards removing the TODO and keeping the naming as is for the following reasons:

What do others think?

@bachase
Copy link
Collaborator

bachase commented Jul 5, 2018

I'm actually fine changing these to follow naming conventions in the code. I was stickier in the past during the initial refactor, but don't see as much need anymore. These are treated as constant by the generic consensus code, so changing that behavior at least requires some intent.

@scottschurr
Copy link
Collaborator

Personally, I'm good with leaving the names unchanged. The names currently have a lower case preface, which is a common way to indicate non-macro-constants in this code base. So it seems that a good path forward would be to simply remove the TODO.

@JoeLoser
Copy link
Contributor Author

JoeLoser commented Jul 6, 2018

Thanks for the explanations and reasoning, guys. Based on the TER codes, it seems that the current coding conventions are exacly as @scottschurr pointed out.

I'm happy to simply remove the TODO and move on.

@ripplelabs-jenkins
Copy link
Collaborator

Thank you for your submission. It will be reviewed soon and submitted for processing in CI.

@JoeLoser JoeLoser changed the title Rename data members of ConsensusParms to not look like macros Remove TODO in DisputedTx.h about renaming constants for ConsensusParms Jul 6, 2018
@codecov-io
Copy link

Codecov Report

Merging #2614 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2614      +/-   ##
===========================================
- Coverage    70.83%   70.81%   -0.02%     
===========================================
  Files          698      698              
  Lines        54367    54367              
===========================================
- Hits         38510    38502       -8     
- Misses       15857    15865       +8
Impacted Files Coverage Δ
src/ripple/consensus/DisputedTx.h 97.26% <ø> (ø) ⬆️
src/ripple/server/impl/BaseHTTPPeer.h 87.65% <0%> (-3.09%) ⬇️
src/ripple/ledger/impl/ApplyView.cpp 80% <0%> (-2.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 381a1b9...ab65c47. Read the comment docs.

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍 Thanks for helping remove some noise.

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@wilsonianb wilsonianb added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 10, 2018
@nbougalis
Copy link
Contributor

Merged with #2629 as 79d8195. Thanks for your contribution, Joe.

@nbougalis nbougalis closed this Jul 27, 2018
@JoeLoser JoeLoser deleted the consensus_parms branch July 31, 2018 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants