stream_info: Collapse constructors#10691
Conversation
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
| std::string route_name_; | ||
|
|
||
| private: | ||
| StreamInfoImpl(absl::optional<Http::Protocol> protocol, TimeSource& time_source) |
There was a problem hiding this comment.
Do you need this intermediate constructor? Can the 2 arg constructor (with an optional protocol arg) call the private 3 arg one directly?
There was a problem hiding this comment.
If I understand the question correctly - I wanted to avoid changing the public API of the struct.
It's possible to collapse StreamInfoImpl(TimeSource& time_source) and StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source) into StreamInfoImpl(TimeSource& time_source, absl::optional<Http::Protocol> protocol = absl::nullopt) and fix all call sites if that's what you're asking. After that we have 2 public constructors with both of them calling into single private one.
There was a problem hiding this comment.
Another alternative is to just duplicate std::make_shared<FilterStateImpl>(FilterState::LifeSpan::FilterChain) call in first two constructors, wdyt?
There was a problem hiding this comment.
Agree, not worth it to change callsites just to flip args and have the default. I was just suggesting making the public 2 arg constructor directly be StreamInfoImpl(absl::optional<Http::Protocol> protocol, TimeSource& time_source), since implicit conversion seems to work and you wouldn't need to change callsites to use absl::make_optional for the protocol. But stylisticly it's seems clearer to be explicit when using optional.
Happy with the latter, that sounds like a reasonable balance of clarity and simplicity!
There was a problem hiding this comment.
Oh, I see. I don't have a strong preference, let me update the pr to change the signature.
There was a problem hiding this comment.
Sorry for being unclear -- i think that your suggestion here
Another alternative is to just duplicate std::make_shared(FilterState::LifeSpan::FilterChain) call in first two constructors, wdyt?
Is most clear since I think most uses of optional are explicitly with make_optional in the codebase. Sorry about that -- should be good to go after that!
…-constructor Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
…-constructor Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
@asraa Friendly ping. |
…-constructor Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
@asraa @mattklein123 PTAL or recommend a reviewer, this PR is awaiting a review for 10 days now. |
asraa
left a comment
asraa
left a comment
There was a problem hiding this comment.
LGTM! I'm really sorry for the delay, this fell off my radar. Feel free to bug me on slack!
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com> Signed-off-by: pengg <pengg@google.com>
3 participants
Description: Reduce code duplication in StreamInfo by chained calls to other constructors
Risk Level: low (code restructure, no behavior changes)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a