jamulussoftware
Add some missing const refs to CHostAddress params
Short description of changes
Refactoring: pass CHostAddress read-only parameters as const references instead of by value, except for signals and slots.
CHANGELOG: Internal: changed passing of some CHostAddress parameters to be const references.
Context: Fixes an issue?
In the current code base, there are many instances where a CHostAddress is already passed as const CHostAddress&, but some instances where a CHostAddress object is passed by value instead. Depending on the platform, passing a reference may be more efficient than passing a whole object by value. This PR catches a few pass-by-value instances that can better be passed as const refs. However, when passing to signal or slot functions, pass-by-value is needed for thread safety, so those occurrences have not been changed.
The only functions that need to receive a non-const CHostAddress& are the ones for parsing addresses, as they pass the results of the parsing back to the caller via the reference.
Does this change need documentation? What needs to be documented and how?
Probably not.
Status of this Pull Request
Compiled fine and under test.
What is missing until this pull request can be merged?
Review, and running for a while on a server.
Checklist
- [x] I've verified that this Pull Request follows the general code principles
- [x] I tested my code and it does what I want
- [x] My code follows the style guide
- [x] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
- [x] I've filled all the content above
Yep, I guess my only concern is if everything is a reference, then somewhere (untracked?) there's the original data... which could get freed and thus the references are no longer valid. There must be somewhere the actual value gets stored...
Yep, I guess my only concern is if everything is a reference, then somewhere (untracked?) there's the original data... which could get freed and thus the references are no longer valid. There must be somewhere the actual value gets stored...
I think the only concern would be if the receiving function is (relatively) long-lived, and the variable of which the reference is passed could change value in a different thread. For passing an unchanging value to a function that will just use it and return, passing a reference to the original parameter would be fine.
I'll do a pass through the modified calls to check there are no dodgy possibilities.
I've reverted most of the changes, specifically the ones relating to signals and slots. Connections from signals to slots can cross thread boundaries, so the receiver doesn't want to be referring to data belonging to the sender. So these should remain as call-by-value, ensuring the receiver has its own copy of the parameter. My original changes probably worked ok for a client, but would be liable to breakage in a server with multiple connections, particularly with multi-threading.
That leaves only a few changes remaining. I still need to double-check CChannel::PutAudioData(), but I think that one is ok. The other changes that remain are just immediate uses for initialisation, which is fine.
Also removed a couple of unused members from CSocket and moved RecHostAddr from the class to OnDataReceived(), as it is only used in that function and doesn't need to persist.
I need to update the original PR description above, as it is now rather out of date.
I still need to double-check
CChannel::PutAudioData(), but I think that one is ok.
Yes, CChannel::PutAudioData() is fine being changed to take a const ref. CServer::PutAudioData() already does so.
Could you squash before the merge, please.
I'll see if I can get this onto my Servers and Directories. <edit>DONE</edit>
I may run it quickly ASAP to check for something obvious too but otherwise it's probably ok then.
