Reset QuicheMemSliceImpl() in move assignment operator.

[bencebeky]

Signed-off-by: Bence Béky bnc@google.com

Commit Message: Reset QuicheMemSliceImpl() in move assignment operator.
Additional Description: minor tweak
Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

[RyanTheOptimist]

This LGTM, but as long as you're here it would be awesome to include some of the digging you did into this bug into a comment. Also would it be possible to add a regression test for this bug to envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc?

[bencebeky]

This LGTM, but as long as you're here it would be awesome to include some of the digging you did into this bug into a comment. Also would it be possible to add a regression test for this bug to envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc?

Thank you for the review.

I'm planning to add a test to quiche/common/platform/api/quiche_mem_slice_test.cc in upstream QUICHE. I couldn't do it before fixing the bug because it would otherwise block rolling QUICHE into Envoy. This behavior is not implementation-specific, and every QUICHE embedder deserves a test. In my mind envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc should only be used for tests that are specific to Envoy's platform implementations (or use test machinery that is specific to Envoy), and not for testing platform API in general.

[RyanTheOptimist]

This LGTM, but as long as you're here it would be awesome to include some of the digging you did into this bug into a comment. Also would it be possible to add a regression test for this bug to envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc?

Thank you for the review.

I'm planning to add a test to quiche/common/platform/api/quiche_mem_slice_test.cc in upstream QUICHE. I couldn't do it before fixing the bug because it would otherwise block rolling QUICHE into Envoy. This behavior is not implementation-specific, and every QUICHE embedder deserves a test. In my mind envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc should only be used for tests that are specific to Envoy's platform implementations (or use test machinery that is specific to Envoy), and not for testing platform API in general.

Sounds great. Thanks!

[RyanTheOptimist]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20997 (comment) was created by @RyanTheOptimist.

see: more, trace.

[bencebeky]

This LGTM, but as long as you're here it would be awesome to include some of the digging you did into this bug into a comment. Also would it be possible to add a regression test for this bug to envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc?

Thank you for the review.
I'm planning to add a test to quiche/common/platform/api/quiche_mem_slice_test.cc in upstream QUICHE. I couldn't do it before fixing the bug because it would otherwise block rolling QUICHE into Envoy. This behavior is not implementation-specific, and every QUICHE embedder deserves a test. In my mind envoyproxy/envoy/test/common/quic/platform/quic_platform_test.cc should only be used for tests that are specific to Envoy's platform implementations (or use test machinery that is specific to Envoy), and not for testing platform API in general.

Sounds great. Thanks!

I just added the tests to QUICHE at https://quiche.googlesource.com/quiche/+/e5c727cc55dcd855a44034424c84b4a4a7d1135a. Note that Envoy build files have a minor bug causing these tests not to run, so this does not block QUICHE roll. I'm fixing that bug at #21013.