[multicapture 1/3] Refactor to extract useful functions/constants for multi-capture#837
[multicapture 1/3] Refactor to extract useful functions/constants for multi-capture#837Arpafaucon wants to merge 2 commits into
Conversation
Arpafaucon
force-pushed
the
multicapture/refactor-common
branch
from
b960872 to
b9323c1
Compare
|
ran clang-format on it (only on the changes ) |
wolfpld
force-pushed
the
master
branch
4 times, most recently
from
2511616 to
aa451b4
Compare
| int port = 8086; | ||
| int seconds = -1; | ||
| int64_t memoryLimit = -1; | ||
| int64_t memoryLimit = tracy::NO_WORKER_MEMORY_LIMIT; |
There was a problem hiding this comment.
What is the purpose of this change? How is writing or reading this much text more clear in intent than a negative value?
There was a problem hiding this comment.
the point was less to clarify the constant than being able to see where this logic is used in other files. There's a lot -1 constants in the codebase, but renaming it enabled to find all the places we were talking about memory limits.
This is not adding any feature, and you're the maintainer here, so if you prefer tracy::NO_LIMIT or even me keeping several -1 around I can do that. (remark applies to most of the proposed changes here - their goal was to limit code duplication where I understood it well enough to operate changes)
| static std::unique_ptr<tracy::View> view; | ||
| static tracy::BadVersionState badVer; | ||
| static uint16_t port = 8086; | ||
| static uint16_t port = tracy::DEFAULT_BROADCAST_UDP_PORT; |
There was a problem hiding this comment.
What is the point of making this a value defined elsewhere? (Where? I now have to actively seek for it.)
There was a problem hiding this comment.
I was extracting it because we use it in multicapture as well (3 times IIRC). As this was currently in a .cpp file of profiler I can't reference it from elsewhere.
I put it in TracyProtocol.hpp, that felt the best location to store protocol-related configuration.
[I can revert this extraction and use the value directly if you prefer - but I was trying to avoid having multiple 8096 lying around]
| auto ai = (const struct sockaddr_in*)&addr; | ||
| #endif | ||
| inet_ntop( AF_INET, &ai->sin_addr, m_text, 17 ); | ||
| inet_ntop(AF_INET, &ai->sin_addr, m_text, TEXT_SIZE); |
There was a problem hiding this comment.
this constant is being used in multicapture 3 times. I thought best to extract it to give it a name.
I agree TEXT_SIZE is not a good name; maybe you have a suggestion
[as other extraction changes in this MR, I can revert it - but I'll have to copy-paste its value where it's used, and that felt less clean and understandable when diving into the codebase]
| return std::nullopt; | ||
| } | ||
|
|
||
| uint64_t ClientUniqueID(tracy::IpAddress const& addr, uint16_t port) |
There was a problem hiding this comment.
I assume the client cannot connect with different IP addresses and ports along its lifetime, so this info can uniquely identify him ? Multicapture uses this as a key to the map of clients, so knowing if it's unique was important.
that said, ClientID works too - I can change that if you prefer
2 participants
Part of #825 , extracted because it can be merged, reviewed and tested separately