framework: Mark subvector and supervector final

[jwnimmer-tri]

This will make the h/cc split clearer in the future (#12814), as well as allow us to improve the implementations and inheritance tree of the VectorBase family.

This is a breaking change, in the unlikely event that anyone was inheriting from these classes.

Also deprecate a worthless constructor of Subvector.

---

This change is 

[jwnimmer-tri]

+@sherm1 WDYT?

[sherm1]

See comments below. I like the final and nuking the weird constructor but not the change to ContinuousState.

Reviewed 7 of 7 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

systems/framework/continuous_state.cc, line 18 at r1 (raw file):

  state_ = std::move(state);
  generalized_position_.reset(new BasicVector<T>());
  generalized_velocity_.reset(new BasicVector<T>());

There is a semantic difference between this change and the original. While the q and v partitions are empty in both cases, they were previously 0-length subvectors of the full state vector -- in theory you could ask even these 0-length objects what VectorBase was their parent. I can't see any actual problem caused by this change but I think the previous invariant that q, v, and z were all partitions of state was cleaner, even if there were some 0-size partitions. I would prefer Subvector<T>(state.get(), 0, 0) here to make it clear what this is.
That would also make the code for each of the three partitions here parallel.

---

systems/framework/subvector.h, line 48 at r1 (raw file):

  DRAKE_DEPRECATED("2020-12-01",
      "Use BasicVector's default constructor, instead.")
  explicit Subvector(VectorBase<T>* vector)

FYI Per my comment above I still think a zero-length partition is a reasonable object. However, it's an odd beast and I don't think it deserves its own constructor so I'm happy with this deprecation.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

---

systems/framework/continuous_state.cc, line 18 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

There is a semantic difference between this change and the original. While the q and v partitions are empty in both cases, they were previously 0-length subvectors of the full state vector -- in theory you could ask even these 0-length objects what VectorBase was their parent. I can't see any actual problem caused by this change but I think the previous invariant that q, v, and z were all partitions of state was cleaner, even if there were some 0-size partitions. I would prefer Subvector<T>(state.get(), 0, 0) here to make it clear what this is.
That would also make the code for each of the three partitions here parallel.

Done.

[jwnimmer-tri]

+@SeanCurtis-TRI for platform review, please.

[sherm1]

Thanks, feature

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

[SeanCurtis-TRI]

Reviewed 5 of 7 files at r1, 2 of 2 files at r2.
Reviewable status: 2 unresolved discussions (waiting on @jwnimmer-tri)

---

systems/framework/continuous_state.cc, line 17 at r2 (raw file):

ContinuousState<T>::ContinuousState(std::unique_ptr<VectorBase<T>> state) {
  state_ = std::move(state);
  generalized_position_.reset(

BTW Not a defect, but seems like an unnecessary new line on these two statements. Is it so all three "resets" have a similar whitespace pattern?

---

systems/framework/subvector.h, line 51 at r2 (raw file):

      : Subvector(vector, 0, 0) {}

  int size() const override { return num_elements_; }

BTW (meta) We have no standard on this, so I'll just mention a passing opinion. A final class that uses the override keyword seems to send mixed messages.

[jwnimmer-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),sherm1(platform) (waiting on @SeanCurtis-TRI and @sherm1)

---

systems/framework/continuous_state.cc, line 17 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Not a defect, but seems like an unnecessary new line on these two statements. Is it so all three "resets" have a similar whitespace pattern?

Yes, whitespace to keep the lines visually pattern-matched.