Add [[nodiscard]] to symbolic::Polynomial functions.

[hongkai-dai]

This change is 

[jwnimmer-tri]

We should probably disable Werror=unused-result for unit test code. It would be something of this sort:

drake/tools/skylark/drake_cc.bzl

Lines 62 to 66 in 925e382

	# The GCC_CC_TEST_FLAGS will be enabled for all cc_test rules in the project
	# when building with gcc.
	GCC_CC_TEST_FLAGS = [
	"-Wno-unused-parameter",
	]

[hongkai-dai]

Thanks Jeremy!

+@jwnimmer-tri for both reviews please, thanks!

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

[jwnimmer-tri]

feature.

Probably worth a second platform reviewer once we clear feature review. This is somewhat of a novel idea.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers (waiting on @hongkai-dai and @jwnimmer-tri)

---

common/BUILD.bazel line 1307 at r2 (raw file):

    copts= [
        # Allow some of the returned argument to be unused.
        "-Wno-unused-result",

Working

This should probably be project-wide, not just for this one test file. I'm polling on slack now.

---

common/symbolic_polynomial.h line 307 at r2 (raw file):

/// Unary minus operation for polynomial.
Polynomial operator-(const Polynomial& p);

These (here and below .. down through pow) all should probably be [[nodiscard]] as well? They all return a freshly-computed values, and therefore are useless to call for mutable side-effects.

---

common/symbolic_polynomial.h line 522 at r2 (raw file):

                  Derived::ColsAtCompileTime, 0, Derived::MaxRowsAtCompileTime,
                  Derived::MaxColsAtCompileTime>>
Evaluate(const Eigen::MatrixBase<Derived>& m, const Environment& env) {

nit [[nodiscard]]?

---

common/symbolic_polynomial.h line 531 at r2 (raw file):

/// @pre {@p vars is non-empty}.
/// @pydrake_mkdoc_identifier{polynomial}
MatrixX<Polynomial> Jacobian(const Eigen::Ref<const VectorX<Polynomial>>& f,

nit [[nodiscard]]?

[jwnimmer-tri]

Note that CI is failing on the bindings.

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers (waiting on @hongkai-dai and @jwnimmer-tri)

---

common/BUILD.bazel line 1307 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

This should probably be project-wide, not just for this one test file. I'm polling on slack now.

After #17341 merges, this PR should drop this flag. There will be one more EXPECT_THROW warning still remaining. Adding unused() around that one site will fix the problem.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers (waiting on @hongkai-dai)

---

common/BUILD.bazel line 1307 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

After #17341 merges, this PR should drop this flag. There will be one more EXPECT_THROW warning still remaining. Adding unused() around that one site will fix the problem.

That's merged. Over to you for a rebase.

[hongkai-dai]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

common/BUILD.bazel line 1307 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

That's merged. Over to you for a rebase.

Done. Thanks for filing the PR #17341

---

common/symbolic_polynomial.h line 307 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

These (here and below .. down through pow) all should probably be [[nodiscard]] as well? They all return a freshly-computed values, and therefore are useless to call for mutable side-effects.

Done. Good call.

[jwnimmer-tri]

-(release notes: none) +(release notes: yes) because users might notice new warnings from their own code.

Reviewed 2 of 4 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @hongkai-dai)

[jwnimmer-tri]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @hongkai-dai)

[jwnimmer-tri]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @hongkai-dai)

[hongkai-dai]

+@sherm1 for Wednesday's platform review since Jeremy is Tuesday's platform reviewer.

Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

[sherm1]

I'm curious what was the motivation for this change?

But I have no objection -- platform

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform) (waiting on @hongkai-dai)

[jwnimmer-tri]

I assume something along the lines of https://twitter.com/stephantlavavej/status/1151977361207771136. Generally about 80% of functions that return a value should have [[nodiscard]]. It's a terrific bug-catcher.

[hongkai-dai]

I'm curious what was the motivation for this change?

I was calling a function Polynomial::RemoveSmallCoefficients(tol) and forgot to return the value (I thought this function does changes the polynomial in-place).

[sherm1]

I was calling a function Polynomial::RemoveSmallCoefficients(tol) and forgot to return the value (I thought this function does changes the polynomial in-place).

Thanks. Great idea to fix this and related functions prophylactically!