Project

General

Profile

Task #2861

import an implementation of c++17 std::optional

Added by Mark Abraham 8 months ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
core library
Target version:
Difficulty:
uncategorized
Close

Description

There was a recent master-branch bug introduced by mismatches between iterator types and pointers (see, I think, https://gerrit.gromacs.org/#/c/8933/24/src/gromacs/topology/residuetypes.cpp) which has provoked unrelated changes to how we implement ArrayRef::iterator.

The logic leading to the bug was that the return value of std::find_if being returned from a helper function and then immediately compared for whether the value was the end() of the range. However, this is a classic use case for the c++17 std feature optional<T>, where the return type of such a method consumes at least one more byte (for a boolean that represents whether a valid T is found, but has a clear semantic for "do you have a value for me to use?"

I suggest we import a c++14 implementation of such a type (e.g. part of https://github.com/TartanLlama/optional/blob/master/tl/optional.hpp, or https://github.com/akrzemi1/Optional, or https://github.com/martinmoene/optional-lite), and document in the dev guide intended use cases (e.g. like https://www.bfilipek.com/2018/06/optional-examples-wall.html, but notably not including error handling). This will probably find several applications in parsing user input where a thing may or may not be present. We should choose an implementation that works on all our compilers, and e.g. does not require heap access. Any other requirements?

It might also provide a path to a better implementation of optional command-line parameters, such that we do not have to get involved with choosing a number within a domain that encodes that the user didn't ask for a specific behaviour, because that is confusing to use and difficult to test and implement.

We could implement that as gmx::compat::optional<T>, following the same pattern as we had when we imported an implementation of c++14 std::make_unique<T>

Associated revisions

Revision f3689081 (diff)
Added by Mark Abraham 6 months ago

Create gmx::compat::optional

This C++17 std component is an extremely useful vocabulary type that
has a straightforward implementation in C++14. Having it available is
likely to mean we can write simpler, less buggy code in less developer
time. It should be used in cases where the absence of a proper value
has an obvious natural interpretation, e.g. the type of the return
value of a function that parses an integer value from a string, or
searching for a matching element in a range, or optionally naming
a residue type.

Updated developer guide and COPYING file accordingly.

Used compat::optional for some aspects of residue handling. This
simplifies some code, and sometimes avoids needing to make a dummy
residue name string that then needs to be compared with other strings.

Let .clang-tidy file's header management apply to source fields for
test binaries.

Excluded the external code from Doxygen checking. Noted where to go
for documentation in the file Doxygen.

Fixes #2861

Change-Id: Ife34983f234549702902a0ea45dc3592098e377d

History

#1 Updated by Mark Abraham 8 months ago

Some people/implementations go further with monad-like extensions, which we probably don't get enough value from to consider at this time.

#2 Updated by Roland Schulz 8 months ago

We should consider having not just optional but also expected. In many cases when you don't have a value something went wrong in which case you should add an error and not just return an empty optional (and we can't always use exceptions in case of error). Just adding optional might encourage us to skip on the error because there is no convenient way to express it.

#3 Updated by Gerrit Code Review Bot 8 months ago

Gerrit received a related patchset '2' for Issue #2861.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~Ife34983f234549702902a0ea45dc3592098e377d
Gerrit URL: https://gerrit.gromacs.org/9273

#4 Updated by Mark Abraham 8 months ago

Roland Schulz wrote:

We should consider having not just optional but also expected. In many cases when you don't have a value something went wrong in which case you should add an error and not just return an empty optional (and we can't always use exceptions in case of error). Just adding optional might encourage us to skip on the error because there is no convenient way to express it.

I'm definitely open to that. https://github.com/martinmoene/optional-lite seems preferable to https://github.com/TartanLlama/expected. It implements revision 7 of the proposal, and doesn't bundle a bunch of extensions to the proposal that we don't want to adopt in GROMACS.

#5 Updated by Gerrit Code Review Bot 7 months ago

Gerrit received a related patchset '1' for Issue #2861.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I5f0cb64bf96bfbf75d092cdd4a2c1d631f0aec08
Gerrit URL: https://gerrit.gromacs.org/9379

#6 Updated by Mark Abraham 6 months ago

  • Status changed from New to Resolved

#7 Updated by Mark Abraham 6 months ago

  • Status changed from Resolved to Closed
  • Target version changed from 2020 to future

Also available in: Atom PDF