Add some missing const refs to CHostAddress params #3259

softins · 2024-04-02T16:50:35Z

Short description of changes

Refactoring: pass CHostAddress read-only parameters as const references instead of by value, except for signals and slots.

CHANGELOG: Refactor 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

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

src/channel.h

src/socket.cpp

pljones · 2024-04-16T17:07:33Z

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...

softins · 2024-04-16T17:21:04Z

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.

ann0see

Not sure how much I can say here, but if it runs correctly, it should be fine? Yes, multithreading could be a possible source of incorrectness?

softins · 2024-04-19T21:30:59Z

I'm off on holiday for a week. I'll pick it up again when I get back.

softins · 2024-05-14T21:29:21Z

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.

RecHostAddr doesn't need to be a class member, as it is only used in CSocket::OnDataReceived(), so make it local.

softins · 2024-05-14T21:38:50Z

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.

softins · 2024-05-14T21:47:10Z

I need to update the original PR description above, as it is now rather out of date.

src/socket.cpp

softins · 2024-05-15T15:24:44Z

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.

pljones · 2024-05-15T15:25:59Z

Overall, there's no problems now, I think.

pljones · 2024-05-16T15:53:30Z

Could you squash before the merge, please.

I'll see if I can get this onto my Servers and Directories. <edit>DONE</edit>

ann0see · 2024-05-16T21:42:47Z

I may run it quickly ASAP to check for something obvious too but otherwise it's probably ok then.

ann0see · 2024-05-25T21:09:19Z

src/recorder/jamrecorder.cpp

@@ -41,7 +41,7 @@ using namespace recorder;
 * Creates a file for the raw PCM data and sets up a QDataStream to which to write received frames.
 * The data is stored Little Endian.
 */
-CJamClient::CJamClient ( const qint64 frame, const int _numChannels, const QString name, const CHostAddress address, const QDir recordBaseDir ) :


Please check for bugs here too.

I tested running the server on macOS and it needed multiple tries to show up as directory. The directory did not show up for a local client if a path for recordings is set. It may not be related and needs more investigation (test with a path that is not writable).

It seems as if on 3.10.0 the directory shows up instantly.

Also the Now a directory text doesn't show up in green:

Hmm. Ok. Current main also shows this issue. Seems to be unrelated.

OK, if this problem behaviour is repeatable post-3.10.0 but not in 3.10.0, it needs to be raised as a new issue. Maybe need to bisect to find where it was introduced?

Issue opened

ann0see

Otherwise everything seems ok (testing to connect to server, listening, audio goes both ways)

softins added the refactoring Non-behavioural changes, Code cleanup label Apr 2, 2024

softins added this to the Release 3.11.0 milestone Apr 2, 2024

Add some missing const refs to CHostAddress params

a1d7c98

softins force-pushed the const-ref branch from d6b9e43 to a1d7c98 Compare April 2, 2024 16:56

softins requested review from pljones and ann0see April 15, 2024 11:34

pljones reviewed Apr 16, 2024

View reviewed changes

src/channel.h Show resolved Hide resolved

pljones reviewed Apr 16, 2024

View reviewed changes

src/socket.cpp Outdated Show resolved Hide resolved

ann0see reviewed Apr 18, 2024

View reviewed changes

pljones assigned softins May 6, 2024

softins marked this pull request as draft May 14, 2024 17:22

Revert changes for signal/slot parameters

fc39452

Remove unused vars from CSocket and move RecHostAddr

a49ae9e

RecHostAddr doesn't need to be a class member, as it is only used in CSocket::OnDataReceived(), so make it local.

pljones reviewed May 15, 2024

View reviewed changes

src/socket.cpp Show resolved Hide resolved

pljones reviewed May 15, 2024

View reviewed changes

src/socket.cpp Show resolved Hide resolved

pljones reviewed May 15, 2024

View reviewed changes

src/socket.cpp Show resolved Hide resolved

Move comment to better place

05c6e01

softins marked this pull request as ready for review May 15, 2024 17:31

pljones approved these changes May 16, 2024

View reviewed changes

softins requested a review from ann0see May 16, 2024 16:45

ann0see reviewed May 25, 2024

View reviewed changes

ann0see approved these changes May 25, 2024

View reviewed changes

softins merged commit bb1639a into jamulussoftware:main May 25, 2024
11 checks passed

softins deleted the const-ref branch May 25, 2024 21:54

ann0see mentioned this pull request Jun 7, 2024

Server de-registers on GUI state change #3287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some missing const refs to CHostAddress params #3259

Add some missing const refs to CHostAddress params #3259

softins commented Apr 2, 2024 •

edited by pljones

Loading

pljones commented Apr 16, 2024

softins commented Apr 16, 2024

ann0see left a comment •

edited

Loading

softins commented Apr 19, 2024

softins commented May 14, 2024 •

edited

Loading

softins commented May 14, 2024

softins commented May 14, 2024

softins commented May 15, 2024

pljones commented May 15, 2024

pljones commented May 16, 2024 •

edited

Loading

ann0see commented May 16, 2024

ann0see May 25, 2024 •

edited

Loading

ann0see May 25, 2024

ann0see May 25, 2024

softins May 25, 2024

ann0see Jun 7, 2024

ann0see left a comment

Add some missing const refs to CHostAddress params #3259

Add some missing const refs to CHostAddress params #3259

Conversation

softins commented Apr 2, 2024 • edited by pljones Loading

Checklist

pljones commented Apr 16, 2024

softins commented Apr 16, 2024

ann0see left a comment • edited Loading

Choose a reason for hiding this comment

softins commented Apr 19, 2024

softins commented May 14, 2024 • edited Loading

softins commented May 14, 2024

softins commented May 14, 2024

softins commented May 15, 2024

pljones commented May 15, 2024

pljones commented May 16, 2024 • edited Loading

ann0see commented May 16, 2024

ann0see May 25, 2024 • edited Loading

Choose a reason for hiding this comment

ann0see May 25, 2024

Choose a reason for hiding this comment

ann0see May 25, 2024

Choose a reason for hiding this comment

softins May 25, 2024

Choose a reason for hiding this comment

ann0see Jun 7, 2024

Choose a reason for hiding this comment

ann0see left a comment

Choose a reason for hiding this comment

softins commented Apr 2, 2024 •

edited by pljones

Loading

ann0see left a comment •

edited

Loading

softins commented May 14, 2024 •

edited

Loading

pljones commented May 16, 2024 •

edited

Loading

ann0see May 25, 2024 •

edited

Loading