Remove nested boost::optionals in ISISReflectometry

[adriazalvarez]

Description of work

This PR initially changed some nested boost::optional found in few places of ISISReflectometry interface. In removing those and changing to std::optional, many other boost::optional were dependent, so I've ended up removing most boost::optional in the ISISReflectometry interface.

Fixes #39146.

Further detail of work

• Most nested boost::optional were found either in the LookupValidatorRow or ValidateRow. The nested boost optional were used to flag the state of cells in the rows used in reflectometry. The cell could be in an invalid state (outer boost optional empty), or the cell could be empty but in a valid state (outer boost optional non empty, inner boost optional empty). Looking carefully at the code, I think we can get away with removing most nested boost::optionals, as the validator is already storing the invalid states in the m_invalidColumns vector. As the validator is created at runtime and destroyed just after validation, there is no need for persistent state of m_invalidColumns, so just checking that this is not empty at the end of validation gives enough indication of any invalid state cell. There is a couple of cases in which it was not possible to completely remove the nested boost::optional, in those case a typedef containing a pair <std::optional<T>, bool> was used, with the bool signaling if the cell is in a valid state.

• Most other boost::optional to std::optional changes were straightforward, but pay particular attention to boost::optional<T&> as these cannot be changed to std::optional<T&>. This seems to be somewhat a contentious issue but it seems an OK workaround is to use std::optional<std::reference_wrapper<T>>, so that's the solution I have opted for.

To test:

• Test coverage in ISISReflectometry is quite exhaustive, all tests passing should be a good indication everything is in order.
• Complete the manual testing for Reflectometry and make sure there are no bugs related to these changes: https://developer.mantidproject.org/Testing/ReflectometryGUI/ReflectometryGUITests.html
• Do the initial part of the ISISReflectometry tutorial (particularly the usage of the LookupTable): https://docs.mantidproject.org/nightly/interfaces/reflectometry/ISIS%20Reflectometry.html

---

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

• Is the code of an acceptable quality?
• Does the code conform to the coding standards?
• Are the unit tests small and test the class in isolation?
• If there is GUI work does it follow the GUI standards?
• If there are changes in the release notes then do they describe the changes appropriately?
• Do the release notes conform to the release notes guide?

Functional Tests

• Do changes function as described? Add comments below that describe the tests performed?
• Do the changes handle unexpected situations, e.g. bad input?
• Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

[peterfpeterson]

I bet you can now removed boost::optional from python/mantidqt/sip/ now too (header and source)

[MialLewis]

Thanks for this PR.

There are one nice refactors that make the code cleaner and easier to understand.

A few requests/queries.

[MialLewis]

Thanks for the changes, this is looking good.

Can you raise issues for the following:

• remove boost::optional from python/mantidqt/sip/
• Re-add std::optionals to clipboard and remove helper function when upstream changes have been made,

[sf1919]

Please don't gatekeep this one until those issues have been raised and link to this PR for reference.

[adriazalvarez]

Issue for remaining boost::optional: #39253

[jclarkeSTFC]

Looks great, interface worked fine for me. I just had one question about a test.

[jclarkeSTFC]

Is this line testing the same thing as before? Does it need this extra line?

Suggested change

	TS_ASSERT(parseScaleFactor("").second);
	TS_ASSERT(parseScaleFactor("").second);
	TS_ASSERT_EQUALS(std::nullopt, parseScaleFactor("").first());

[adriazalvarez]

You mean between testing parameters "ABSC" and "". In that case is yes, they are testing two different things. "ABSC" is a wrong scale factor, so the cell should report invalid (the pair returns false). In the other case "", the cell is in a valid state (it's ok to be empty) but there is no scale factor so it is empty.

[jclarkeSTFC]

I mean that previously it was checking that "" returned a boost::none but now it's checking that the validity bool is true, but not that it's a std::nullopt.

[adriazalvarez]

ah yes, before the outer nested boost::optional was wrapping a boost::none to communicate essentially the same, that the cell is in valid state but empty. Anyway, I can add the check for std::nullopt too.

[jclarkeSTFC]

Thanks for adding that, looks good.