[solvers] Add SpecificOptions sugar for solvers to map their options

[jwnimmer-tri]

Towards #21345 (which ports all solvers to use this new sugar) and thus also #20967.

---

This change is 

[jwnimmer-tri]

+@ggould-tri for feature review, please. (If you have time? No particular ETA, maybe within a week is okay.)

[ggould-tri]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, needs at least two assigned reviewers

---

solvers/specific_options.h line 25 at r1 (raw file):

generic SolverOptions struct into the specific solver's options API.

This class is intended a short-lived helper (i.e., a local variable on the call

typo?

Suggestion:

This class is intended as a short-lived helper (i.e., a local variable on the call

---

solvers/specific_options.h line 53 at r1 (raw file):

  participate in the Method 1 or 2 copying / callbacks.

For examples of use, refer to all of the existing Drake solver wrappers. */

nit , a less herculean task.

Suggestion:

any

---

solvers/specific_options.h line 60 at r1 (raw file):

  /* Creates a converter that reads the subset of `all_options` destined for the
  given `id`. Both arguments are aliased, so must outlive this object. */
  SpecificOptions(const SolverId* id, const SolverOptions* all_options);

BTW: I don't love a ctor aliasing default-copyable objects, and ...* id is very odd to read, but I assume that this is justified by callers expecting inner-loop performance.

Code quote:

  given `id`. Both arguments are aliased, so must outlive this object. */
  SpecificOptions(const SolverId* id, const SolverOptions* all_options);

---

solvers/specific_options.h line 153 at r1 (raw file):

  //@}

  // The solver we're operating on behalf of (as passed to our constructor).

nit: Mixing /* and // comment styles make styleguide sad.

---

solvers/specific_options.cc line 149 at r1 (raw file):

  // already been set.
  for (const auto& [respelled_key, boxed_value] : respelled_) {
    // Pedantially, lambdas cannot capture a structured binding so we need to

typo

Suggestion:

Pedantically

[jwnimmer-tri]

+@SeanCurtis-TRI for platform review per schedule, please.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)

---

solvers/specific_options.h line 60 at r1 (raw file):

Previously, ggould-tri wrote…

BTW: I don't love a ctor aliasing default-copyable objects, and ...* id is very odd to read, but I assume that this is justified by callers expecting inner-loop performance.

Correct.

---

solvers/specific_options.h line 153 at r1 (raw file):

Previously, ggould-tri wrote…

nit: Mixing /* and // comment styles make styleguide sad.

The rule is to be consistent, and this is consistent. Functions specifications are /* and implementation notes are //.

[ggould-tri]

Reviewed 2 of 3 files at r2.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)

[ggould-tri]

Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)

[SeanCurtis-TRI]

With some cosmetic stuff.

Reviewed 2 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 7 unresolved discussions

---

solvers/specific_options.h line 29 at r2 (raw file):

convenient tools to map those options into whatever back-end is necessary. Two
different mechanisms are offered, depending on whether the solver's option names
are static or dynamic: CopyToCallbacks or CopyToSerializableStruct.

The ordering of the adjectives doesn't match the ordering of the methods.

Suggestion:

dyanmic or static

---

solvers/specific_options.h line 79 at r2 (raw file):

  /* Returns and effectively removes the value of the option named `key`. (To be
  specific -- `all_options` remain unchanged; instead, the `key` is memorized

btw This always a weird conflict between code and language. all_options is a pointer to a single thing, so should be singular. However, the idea of the options it holds are plural. British English would probably simplify this.

Either we can refer to all_options the C++ object as unchanged, or we can refer to "all options provided at construction remain unchanged", but the mixture is what causes weirdness.

That said, it's basically clear what's being communicated.

Suggestion:

remains 

---

solvers/specific_options.h line 84 at r2 (raw file):

  set, this checks `all_options` for our `id` as well as any options added
  during a Respell().
  @tparam T must be one of: double, int, std::string. */

nit: No note on the possible disagreement between T and the actual type of the option.

---

solvers/specific_options.h line 101 at r2 (raw file):

  /* Helper for "Method 2 - static", per our class overview. Converts options
  when the solver uses an options struct with names known at compile time. The
  `Output` struct must obey Drake's Serialize() protocol, for us to interrogate

nit: The struct formerly known as Output.

Suggestion:

`Result`

---

solvers/specific_options.cc line 279 at r2 (raw file):

              id_->name(), name, value));
        }
        if (static_cast<int64_t>(value) >

BTW What am I missing here? How can a value encoded as an int ever be bigger than the maximum uint32_t value? Is this simply protecting us from the C++ specification's only-lower-bound- language for defining sizeof(int)?

---

solvers/test/specific_options_test.cc line 79 at r2 (raw file):

GTEST_TEST(SpecificOptionsTest, CopyToCallbacksNoop) {
  // This is a code-coverage test to cover the early return no-op.

btw: This doesn't really test the early return. The test result is the same whether there's an early return or not (each for loop that would exercise the callbacks get exercised zero times). All one can really say about this test is that no options implies no problems.

---

solvers/test/specific_options_test.cc line 89 at r2 (raw file):

}

GTEST_TEST(SpecificOptionsTest, CopyToCallbacksEarlyReturn) {

nit: This test is an exact duplicate of the previous (but with a rename).

[SeanCurtis-TRI]

Reviewable status: 8 unresolved discussions

---

solvers/specific_options.h line 104 at r2 (raw file):

  the option names.
  @param [in,out] result The solver's specific options struct; any options not
  mentioned in `solver_options` will remain unchanged. */

nit: unclear reference...should this be all_options?

Code quote:

solver_options

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion

---

solvers/specific_options.h line 29 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The ordering of the adjectives doesn't match the ordering of the methods.

Done.

---

solvers/test/specific_options_test.cc line 79 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

btw: This doesn't really test the early return. The test result is the same whether there's an early return or not (each for loop that would exercise the callbacks get exercised zero times). All one can really say about this test is that no options implies no problems.

Yeah. Maybe the short-circuit isn't that valuable anyway, so I just punted it. We can add it back later if profiling tells us to.

[SeanCurtis-TRI]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform)