Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: use OptRef helper to reduce some boilerplate #14361

Merged
merged 8 commits into from
Dec 21, 2020

Conversation

snowp
Copy link
Contributor

@snowp snowp commented Dec 10, 2020

Makes use of OptRef and adds some helpers to make it easier to go from OptRef to T* and vice versa.

Risk Level: Low
Testing: Existing tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Snow Pettersen <[email protected]>
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great with just a couple of minor nits.

@@ -20,7 +20,7 @@ namespace Envoy {
#define DUMP_DETAILS(member) \
do { \
os << spaces << #member ": "; \
if ((member) != nullptr) { \
if ((member)) { \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the double-parens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem like it, removing

};

template <class T> OptRef<T> makeOptRef(T& ref) { return {ref}; }

template <class T> OptRef<T> makeOptRef(T* ptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this overload feels like it might result in ambiguity at some point in the future (thought it would have to be some kind of pathological case on optional reference to a pointer to get into that state, I think). Maybe call this one makeOptRefFromPtr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point, I'll make the name change

Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Snow Pettersen <[email protected]>
jmarantz
jmarantz previously approved these changes Dec 14, 2020
};

template <class T> OptRef<T> makeOptRef(T& ref) { return {ref}; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doxygen doc for makeOptRef and makeOptRefFromPtr?

Signed-off-by: Snow Pettersen <[email protected]>
jmarantz
jmarantz previously approved these changes Dec 14, 2020
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thank you!

@jmarantz jmarantz merged commit d7cc4aa into envoyproxy:master Dec 21, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Dec 22, 2020
* master: (30 commits)
  Deflaked: Guarddog_impl_test (envoyproxy#14475)
  [fuzz] add fuzz tests for hpack encoding and decoding (envoyproxy#13315)
  [filters] Prevent a filter from sending local reply and continue (envoyproxy#14416)
  oauth2: improving coverage (envoyproxy#14479)
  owners: Change dio email address (envoyproxy#14498)
  macos build: Fix ninja install (envoyproxy#14495)
  http: use OptRef helper to reduce some boilerplate (envoyproxy#14361)
  doc: update test/integration/README.md (envoyproxy#14485)
  server: wait workers to start before draining parent. (envoyproxy#14319)
  api: relax inline_string length limitation in DataSource (envoyproxy#14461)
  oauth: properly stop filter chain when a response was sent (envoyproxy#14476)
  listener: deprecate use_proxy_proto (envoyproxy#14406)
  deps: update cel and remove a patch (envoyproxy#14473)
  preconnect: rename: (envoyproxy#14474)
  coverage: ratcheting limits (envoyproxy#14472)
  grpc mux: fix sending node again after stream is reset (envoyproxy#14080)
  [test] Replace printers_include with printers_lib. (envoyproxy#14442)
  tcp: nodelay in the new pool (envoyproxy#14453)
  test: replace mock_methodn macros with mock_method (envoyproxy#14450)
  tcp: extending tcp integration test (envoyproxy#14451)
  ...

Signed-off-by: Michael Puncel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants