network: address socket interface pointer instead of name #12550
Conversation
Signed-off-by: Florin Coras <[email protected]>
florincoras
requested review from
alyssawilk,
lizan,
mattklein123 and
PiotrSikora
as code owners
Signed-off-by: Florin Coras <[email protected]>
| @@ -47,6 +47,10 @@ std::string friendlyNameFromAbstractPath(absl::string_view path) { | |||
|
|
|||
| } // namespace | |||
|
|
|||
| static inline const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interface) { | |||
There was a problem hiding this comment.
style nit: put this in unnamed namespace above, remove static, doesn't have to be inline?
There was a problem hiding this comment.
It's a C reflex of mine that I use instead of macros. Goal would be to convince the compiler to avoid extra function calls when not needed. But obviously, no strong opinion here ...
There was a problem hiding this comment.
C++ inline isn't guaranteed to be inlined, all are decided by compiler, it's more of working around one-definition rule.
include/envoy/network/address.h
Outdated
| */ | ||
| virtual const std::string& socketInterface() const PURE; | ||
| virtual const Network::SocketInterface* socketInterface() const PURE; |
There was a problem hiding this comment.
let's return reference, it's no longer nullable.
There was a problem hiding this comment.
We'll have to keep the const because of how we pass the SocketInterface pointer to the Address. Is that okay or were you thinking about something more complex?
There was a problem hiding this comment.
ah of couse, const wherever possible :)
Signed-off-by: Florin Coras <[email protected]>
Signed-off-by: Florin Coras <[email protected]>
source/common/network/address_impl.h
Outdated
| } | ||
|
|
||
| std::string friendly_name_; | ||
| std::string socket_interface_; | ||
| const SocketInterface* socket_interface_; |
Signed-off-by: Florin Coras <[email protected]>
Thank you for the review! |
Signed-off-by: Florin Coras [email protected]
Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a