Allow Journal to be copied/moved

[bachase]

I could be convinced this is an undesirable change, but it greatly simplifies some consensus related work I am doing.

[ripplelabs-jenkins]

Jenkins Build Summary

Built from this commit

Built at 20171204 - 22:51:49

Test Results

Build Type	Result	Status
clang.debug.unity	970 cases, 0 failed, t: 379s	PASS ✅
coverage	970 cases, 0 failed, t: 590s	PASS ✅
clang.debug.nounity	968 cases, 0 failed, t: 306s	PASS ✅
gcc.debug.unity	970 cases, 0 failed, t: 415s	PASS ✅
gcc.debug.nounity	968 cases, 0 failed, t: 747s	PASS ✅
clang.release.unity	969 cases, 0 failed, t: 438s	PASS ✅
gcc.release.unity	969 cases, 0 failed, t: 461s	PASS ✅

[codecov-io]

Codecov Report

Merging #2292 into develop will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@             Coverage Diff             @@
##           develop    #2292      +/-   ##
===========================================
- Coverage    70.96%   70.94%   -0.02%     
===========================================
  Files          691      691              
  Lines        51663    51649      -14     
===========================================
- Hits         36661    36644      -17     
- Misses       15002    15005       +3

Impacted Files	Coverage Δ
src/ripple/beast/utility/Journal.h	96.96% <88.88%> (-0.14%)	⬇️
src/ripple/beast/clock/chrono_util.h	82.6% <0%> (-8.7%)	⬇️
src/ripple/app/ledger/PendingSaves.h	93.54% <0%> (-6.46%)	⬇️
src/ripple/ledger/impl/BookDirs.cpp	84% <0%> (-0.32%)	⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp	78.77% <0%> (-0.26%)	⬇️
src/ripple/app/main/Application.cpp	60.7% <0%> (-0.26%)	⬇️
src/ripple/json/impl/json_value.cpp	75% <0%> (-0.22%)	⬇️
src/ripple/app/misc/NetworkOPs.cpp	62.74% <0%> (-0.03%)	⬇️
src/ripple/app/misc/impl/TxQ.cpp	95.78% <0%> (-0.01%)	⬇️
src/ripple/app/paths/cursor/PathCursor.h	100% <0%> (ø)	⬆️
... and 4 more

---

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 090d813...c18a8aa. Read the comment docs.

[scottschurr]

This is not a topic I feel strongly about, but I'd like to do some sanity checking. I think it was Vinnie's position that Journals should be immovable. A Log, on the other hand, is a very lightweight reference to a Journal, that that's what we should be moving around. I'm guessing Vinnie would want to know what you want to do with the Journal that can't be done with a Log.

[bachase]

@scottschurr Could you point me to the Log class you are referencing?

[HowardHinnant]

@scottschurr Or maybe vice-versa? https://github.com/bachase/rippled/blob/febedc480435ddd56cae199dba4923b2a4dc32a7/src/ripple/beast/utility/Journal.h#L50-L52

[scottschurr]

Gaaaah. I should know better than to try to talk about this stuff from memory. Sorry, my mistake. Logs (https://github.com/ripple/rippled/blob/develop/src/ripple/basics/Log.h#L49) is actually fairly heavy weight. The Journal, what you're playing with, is light weight.

If I'd looked at your code, instead of just reading the headline, I'd see that the problem you really want to solve is making the Journal assignable after the fact. They were previously copyable and moveable, but they were not assignable.

Personally, I have nothing against making Journals assignable. But having a Journal carry a pointer to a Sink instead of a reference to a Sink is a disadvantage, since a pointer can be null.

It would be good to see the use cases you're trying to solve before we make the change.

[HowardHinnant]

I've completed a visual once-over of Journal and I believe that the invariant that m_sink always refers to a Sink remains in place (before and after this change). A comment by the m_sink member declaration that an invariant is that it is always non-null would be helpful to add.

• The Journal copy constructor appears to me to be needlessly non-trivial. I recommend having the compiler provide it.

• As it stands now, we have:

1. ~Journal() is user-declared and compiler-provided.
2. Journal (Journal const& other) is user-declared and user-provided, but does the same thing as the compiler-provided copy constructor would do.
3. Journal& operator=(Journal const&) is not user-declared and is compiler-provided.

The implicitly provided copy assignment operator is currently being provided under deprecated behavior since the destructor and copy constructor are user-declared.

I recommend implicitly defaulting all three of these, or explicitly defaulting all three. Either path will eliminate the dependence on deprecated behavior.

[HowardHinnant]

@scottschurr: I believe this is a brief description of Brad's use case: https://ripplelabs.slack.com/archives/C02GE9NBA/p1511279746000806

[seelabs]

👍

[bachase]

I've squashed and made the commit message clearer per @scottschurr's suggestion.

The motivation: I have a type implementing a generic code that needs to log in some cases, but the generic code doesn't know anything about logging. The most convenient approach was for me to store the Journal in the type itself. The type must be assignable. I tried avoiding this design, but couldn't come up with a good alternative.

[scottschurr]

@HowardHinnant: Thanks for the reference. @bachase, sorry for piping up before I really understood what was going on. I'll try to avoid tripping over myself next time. Carry on.

[bachase]

Piping up is good! Please continue to do so when something stands out to you.

[seelabs]

In 0.90.0-b3