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

Add timer start param to Application [RIPD 1405] #2024

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@mellery451
Copy link
Contributor

mellery451 commented Feb 21, 2017

Modify doStart Application method to specify whether or not to start the
DeadlineTimers. Specify inactive timers for jtx::Env Applications and
active timers for standard Applications.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 21, 2017

Codecov Report

Merging #2024 into develop will decrease coverage by -0.05%.
The diff coverage is 44.44%.

@@             Coverage Diff             @@
##           develop    #2024      +/-   ##
===========================================
- Coverage    67.93%   67.89%   -0.05%     
===========================================
  Files          685      685              
  Lines        49591    49594       +3     
===========================================
- Hits         33689    33670      -19     
- Misses       15902    15924      +22
Impacted Files Coverage Δ
src/ripple/app/main/Application.h 100% <ø> (ø)
src/ripple/app/main/Main.cpp 35.6% <0%> (ø)
src/ripple/app/main/Application.cpp 60.86% <50%> (-0.36%)
src/ripple/core/impl/DeadlineTimer.cpp 88.99% <0%> (-8.26%)
src/ripple/app/misc/impl/LoadFeeTrack.cpp 81.53% <0%> (-4.62%)
src/ripple/app/main/LoadManager.cpp 72.6% <0%> (-1.37%)
src/ripple/app/misc/SHAMapStoreImp.cpp 78.81% <0%> (-1.04%)
src/ripple/core/impl/JobQueue.cpp 86.11% <0%> (-0.47%)

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 b7e2a3b...40ed419. Read the comment docs.

@vinniefalco

This comment has been minimized.

Copy link
Contributor

vinniefalco commented Feb 22, 2017

Suggested commit message first line:

Add timer start param to Application (RIPD 1405):
@@ -357,6 +357,7 @@ class ApplicationImp
std::unique_ptr <TxQ> txQ_;
DeadlineTimer m_sweepTimer;
DeadlineTimer m_entropyTimer;
bool m_startTimers;

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Feb 22, 2017

Contributor

Suggested startTimers_

@@ -529,7 +532,7 @@ class ApplicationImp
//--------------------------------------------------------------------------

bool setup() override;
void doStart() override;
void doStart(Application::timers) override;

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Feb 22, 2017

Contributor

There's no need to bloat the code with frivolous aliases for bool. This will do:

void doStart(bool withTimers = true);

This comment has been minimized.

Copy link
@mellery451

mellery451 Feb 22, 2017

Author Contributor

👍 my original impl. was using the bool, but I tried this for readability. I'll switch it back.

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Feb 22, 2017

Contributor

or

void doStart(bool withTimers);
enum class timers
{
active,
inactive

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Feb 22, 2017

Contributor

\eyeroll

@mellery451 mellery451 force-pushed the mellery451:app-timers branch from 6554e8c to 15cdb66 Feb 22, 2017

@mellery451

This comment has been minimized.

Copy link
Contributor Author

mellery451 commented Feb 22, 2017

updated based on comments - forced push to fix the commit message

@HowardHinnant
Copy link
Contributor

HowardHinnant left a comment

With the latest mods looks good to me. Everything tests out fine on macOS.

@mellery451 mellery451 force-pushed the mellery451:app-timers branch from 15cdb66 to 6ab6379 Mar 1, 2017

@mellery451 mellery451 force-pushed the mellery451:app-timers branch 2 times, most recently from dff284b to 40ed419 Mar 1, 2017

@mellery451 mellery451 added the Passed label Mar 6, 2017

Add timer start param to Application (RIPD 1405):
Modify doStart Application method to specify whether or not to start the
DeadlineTimers. Specify inactive timers for jtx::Env Applications and
active timers for standard Applications.

@mellery451 mellery451 force-pushed the mellery451:app-timers branch from 40ed419 to eac274e Mar 9, 2017

@scottschurr scottschurr referenced this pull request Mar 21, 2017

Merged

Develop next #2059

@scottschurr

This comment has been minimized.

Copy link
Contributor

scottschurr commented Mar 21, 2017

Merged to develop as 95aebfc

@mellery451 mellery451 deleted the mellery451:app-timers branch Mar 21, 2017

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.