chore: dragonfly connection refactorings #4434
Conversation
romange
commented
Jan 9, 2025
romange
commented
- Move socket read code into a dedicated function. Remove std:: prefix in the code.
- Add an optional iouring bufring registration. Currently not being used and is disabled by default.
1. Move socket read code into a dedicated function. Remove std:: prefix in the code. 2. Add an optional iouring bufring registration. Currently not being used and is disabled by default.
| @@ -1241,6 +1243,29 @@ void Connection::HandleMigrateRequest() { | |||
| } | |||
| } | |||
|
|
|||
| error_code Connection::HandleRecvSocket() { | |||
There was a problem hiding this comment.
the only change in this file. the code is moved to HandleRecvSocket
There was a problem hiding this comment.
- the boilerplate in RegisterBufRings
| @@ -87,6 +87,7 @@ ABSL_FLAG(bool, migrate_connections, true, | |||
| "happen at most once per connection."); | |||
|
|
|||
| using namespace util; | |||
| using namespace std; | |||
There was a problem hiding this comment.
I am allergic to unqualified lookup unless it's used in generic code as a customization point 😄
Ignore me 😄
There was a problem hiding this comment.
i do not know what it means but I do not think our code has lots of ambiguous names. as long as we enable it locally in cc file I think it's fine.
There was a problem hiding this comment.
Long story short there are different taxonomy rules for looking up names qualified (e.g, std::some_algo etc) vs unqualified (e.g, some_algo). This can lead to many surprises that are amplified in the presence of generic code (templates and their two phase lookup upon parsing and instantiation).
A simple example without templates:
// Omitting include<algorithm> on purpose
std::accumulate(c.begin(), c.end(), adder());
This is easy, the compiler looks within the std namespace and it finds std::accumulate.
Now if you do the same unqualified
accumulate(...)
The compiler will look in namespaces that are associated with the arguments passed in the function call. In our case it's Vector::Iterator. Now here is a little bit of madness: this is NOT platform independend code*. It might work on some machines and it might fail to compile for others. Why?
Because the cpp standard does not mandate if the iterator is just a pointer or if it's wrapper struct. If it's a typedef to an int then guess what! The program won't compile. If the implementation is a struct Iterator it will because the rules mandate that the namespaces for the compiler to search include the namespace of the class. However if it was an int, these namespaces are ignored so the compiler ends up searching in a different way.
Also, because we did not include algorithm we might or might not pull the right std::accumulate, or we might not even consider it (and which header includes what again is not defined by the cpp standard)
This is just without the templates. You get templates and customization points and now you are dancing with tag_dispatch and the infamous two-step dance (using declaration + followed by an unqualified lookup)
There was a problem hiding this comment.
p.s. I am not advocating this here, just left the comment to explain some of the reasons I don't like it.
p.s.2 We also do not use a lot of templates so we are kinda on the safer side 😄
