[solvers] Reformulate SolverOptions.SetOption to use our variant

[jwnimmer-tri]

This simplifies our API surface for users, and makes the implementation of #22078 easier.

Towards #22078.

---

This change is 

[jwnimmer-tri]

+@hongkai-dai would you like to feature-review this?

[hongkai-dai]

Reviewed 4 of 5 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/solver_options.cc line 28 at r1 (raw file):

  std::visit(
      overloaded{
          [&](double& unboxed_value) {

Curious why we use a reference double& instead of const reference here? And why we use std:move(key) instead of const std::string_view& key?

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/solver_options.cc line 28 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Curious why we use a reference double& instead of const reference here? And why we use std:move(key) instead of const std::string_view& key?

The goal is to avoid move the value into place when it's a string. I've respelled the code a bit to hopefully make that more clear?

[hongkai-dai]

Reviewed 1 of 5 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/solver_options.cc line 28 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The goal is to avoid move the value into place when it's a string. I've respelled the code a bit to hopefully make that more clear?

I see. Thanks! Let me rephrase it to make sure I understand

Since we just move value into solver_options_str_, we want to make sure that everything is rvalue, hence we use rvalue reference here.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/solver_options.cc line 28 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I see. Thanks! Let me rephrase it to make sure I understand

Since we just move value into solver_options_str_, we want to make sure that everything is rvalue, hence we use rvalue reference here.

Yes, exactly.

(I believe using the lvalue reference and moving from it (as I did in r1) would also still avoid the copying, but is less clear to a person reading the code. Writing out the && in the signature here most clearly communicate the intention.)

[hongkai-dai]

+@ggould-tri for platform review please, thanks!

Reviewable status: LGTM missing from assignee ggould-tri(platform)

---

solvers/solver_options.cc line 28 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, exactly.

(I believe using the lvalue reference and moving from it (as I did in r1) would also still avoid the copying, but is less clear to a person reading the code. Writing out the && in the signature here most clearly communicate the intention.)

Got it, thanks for the explanation!

[ggould-tri]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),hongkai-dai