conn_pool: cleaning up interfaces#11796
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
WDYT? Better than voids? |
|
For the |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM with small comment.
/wait
source/common/http/conn_pool_base.cc
Outdated
| Http::ResponseDecoder& decoder = *static_cast<const HttpAttachContext*>(&context)->decoder_; | ||
| Http::ConnectionPool::Callbacks& callbacks = | ||
| *static_cast<const HttpAttachContext*>(&context)->callbacks_; |
There was a problem hiding this comment.
Technically I think you should do dynamic_cast here and elsewhere. You might consider a helper that does it and returns the real object, or just a template method on the interface like we for for thread local storage slots.
There was a problem hiding this comment.
Heh, Greg had me go from reinterpret to static. I think static cast is consistent with the style guide*. Maybe I could add ASSERTS for dynamic cast like Greg suggested?
There was a problem hiding this comment.
Sure dynamic_cast in ASSERT SGTM, I would still put it in a templated helper though on the interface. Can you potentially take an action to change the ThreadLocal interface to do the same thing? There might be a nice perf boost there.
There was a problem hiding this comment.
template function added. lmk if it's not what you were looking for.
Sadly when I tried to add the dynamic cast assert it failed because not polymorphic - no virtuals in the base class). I could add some or we could leave as-is.
There was a problem hiding this comment.
Also yeah, happy to do the follow-up with TLS.
There was a problem hiding this comment.
Yes this LGTM, sorry why does the dynamic_cast fail? The virtual destructor doesn't count?
|
|
||
| // A placeholder struct for whatever data a given connection pool needs to | ||
| // successfully attach and upstream connection to a downstream connection. | ||
| struct AttachContext {}; |
There was a problem hiding this comment.
Oh I see, here. Yeah just add a virtual destructor and the dynamic_cast will work? This is what we do for the ThreadLocal code. Up to you.
ggreenway
left a comment
There was a problem hiding this comment.
Yeah, let's add a virtual destructor, then add the dynamic_cast assert.
/wait
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
requested follow-up PR from #11796 as it's more consistent with our style guide. Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
requested follow-up PR from envoyproxy#11796 as it's more consistent with our style guide. Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: scheler <santosh.cheler@appdynamics.com>
Removing two "friend class" declarations by providing necessary accessors.
Replacing void* context with castable context.
Risk Level: Low (no-op refactor)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a