WalkerLogXXX should provide move constructors

[PDoakORNL]

Proposed changes

I see no documented reason for why move constructors or the rule of five in general should be broken for WalkerLog* classes. And it became a subtle use after return asan issue when refactoring batched drivers. I have addressed this sufficiently to address work needed for testing of variable estimator measurements to be sound and testable. I have also added the the beginning of actual unit testing for WalkerLogManager and a testing data class for WalkerLogInput.

There was a nasty shared state reference which so far looks to have been unused. This will all get more coverage int he variable esimator measurement period PR.

What type(s) of changes does this code introduce?

Delete the items that do not apply

• Bugfix
• New feature
• Documentation or build script changes
• Other (please describe):

Does this introduce a breaking change?

• No

What systems has this change been tested on?

Checklist

Update the following with an [x] where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

• • I have read the pull request guidance and develop docs
• • This PR is up to date with the current state of 'develop'
• • Code added or changed in the PR has been clang-formatted
• • This PR adds tests to cover any new code, or to catch a bug that is being fixed
• • Documentation has been added (if appropriate)

[PDoakORNL]

For me this is useless without #5566 but it seems that for the CI machines that PR is not needed to use asan.

[ye-luo]

Does adding default copy, move constructors and assign operator change any code behavior? I though they are already there implicitly. Is there any functional change?

[ye-luo]

IIRC. WalkerLogManager owns and manipulates the state_ and WalkerLogState only reads it. In this way, per crowd WalkerLogState won't go out of sync. For that reason, keeping it a const ref seems fine to me. What is your motivation to make it a copy especially a non-const copy?

[ye-luo]

Sent out review before my laptop died. Continue reading the added test.

[PDoakORNL]

You cannot move a ref.

[ye-luo]

Can you make it at least a const?

[PDoakORNL]

So multiple threads spectate on a single memory location to stay "synchronized"? examining the source shows no evidence this anti feature is used and no crowd level resources state should be getting controlled in this manner. If this needs to be updated then a call can be made to change the behavior of the per crowd loggers.

[ye-luo]

This factory function shows the tight connection between the manager and collectors as it returns them as a whole structure.

We can write the code as

   auto make_stuff = [comm](WalkerLogInput& walker_log_input) -> LogAndStuff {
-    WalkerLogManager wlm{walker_log_input, true, "root_name", comm};
-    CollectorHolder ch;
-    ch.setWalkerLogCollector(wlm.makeCollector());
-    return {std::move(wlm), std::move(ch)};
+    LogAndStuff logger{{walker_log_input, true, "root_name", comm}, {}};
+    logger.ch.setWalkerLogCollector(logger.wlm.makeCollector());
+    return logger;
   };

In this way, the hassle of move constructors can be avoided and also reduce the number copy operations.

The intention of introducing move constructor and operator is to bring real benefit. It is not worth maintaining move constructor and operators rigorously in every class but only places we benefit. Hopefully everyone agrees with that.

[ye-luo]

The above was my opinion. This is not a request for change.
We can keep the code as you changed for now.

[PDoakORNL]

Is there a point in blocking this other than making the refactoring I need to do to implement effective testing for a requested feature harder? I think ease of testing refactoring trumps weasling out of providing sane threaded design and basic constructors.

I'm correcting a design issue that should have never gotten through review the reference used as a back door state update and fixing the lack of clear declaration of constructors.

[ye-luo]

Probably you missed my other comment, I'm not making a request for change in this area of code. The blocking reason is missing CMake changes in this PR. Your added code is not compiled and tested.

To be honest, the name state made me feel it may carry variable like step counters. If that was the case, I wanted it to be a const ref in collectors and let the manager to operate it.
But the source code WalkerLogState really shows it is just parsed input. So copy is fine.

[ye-luo]

Please connect test_WalkerLogManager.cpp in CMake.

[PDoakORNL]

Working on too many things at once, somehow I lost the cmake changes in the shuffle.

[ye-luo]

Test this please