Unnecessary deep-copy of std::vector in method calls?

[tomneda]

Great project, makes really fun working and analyzing it. Thanks for that.

I found several places where the data vector (as a std::vector ) are transferred to a method with a (deep-)copy. As I could see that the content is only read in the methods it would conserve memory and CPU time by using a const-reference for that.

I looked not very deeply through the vast code yet but I found at least these six methods (I tried it out if it works, so I copy the already changed code here, only the headers):

In class ofdmDecoder:

void    processBlock_0      (const std::vector<std::complex<float> > &);
void    decode          (const std::vector<std::complex<float> > &, int32_t n, std::vector<int16_t> &);

In class phaseReference:

int32_t findIndex           (const std::vector<std::complex<float> > &, int);
int16_t estimate_CarrierOffset      (const std::vector<std::complex<float> > &);
float   estimate_FrequencyOffset    (const std::vector<std::complex<float> > &);
float   phase               (const std::vector<std::complex<float>> &, int);

Maybe there are further places?

[JvanKatwijk]

you are right, most vectors were replacements of "XXX *" constructs

[tomneda]

Playing music worked fine in first tests but I saw many outputs of "in frameprocessor" on the console. I debugged and I found an derived interface in method dataProcessor::addtoFrame() which still had the std::vector<uint8_t) without reference symbol. So, the base class of the interface definition in class frameProcessor is called instead of the derived method due to the different parameter signature.

BTW, I would strongly recommend using const together with referenced input parameters in methods. I see this advantages:

1. The caller of the method can see at once that the parameter cannot be changed in the method. So, it could only be an input parameter.
2. The method itself cannot change the parameter (so also the calling parameter) unintentionally.
3. The caller can itself define the transferred variable as const (I am loving using const where ever possible to avoid "careless mistakes").

The only disadvantage of const for me is: a word more has to be written :-) Same is also valid with transferred pointers instead of references for input parameters.

[JvanKatwijk]

As far as I know, the output in the frameprocessor was fixed earlier this week Wrt adding "const". In my opinion a decent optimizing compier does sufficient data f;low analysis to detect whether or not a values is changed in a function, I do not have fundamental objections to using "const", but I do not think it add much to the program

Op vr 19 mei 2023 om 00:20 schreef TomNeda @.***>:

Playing music worked fine in first tests but I saw many outputs of "in frameprocessor" on the console. I debugged and I found an derived interface in method dataProcessor::addtoFrame() which still had the std::vector<uint8_t) without reference symbol. So, the base class of the interface definition in class frameProcessor is called instead of the derived method due to the different parameter signature.

BTW, I would strongly recommend using const together with referenced input parameters in methods. I see this advantages:

1. The caller of the method can see at once that the parameter cannot be changed in the method. So, it could only be an input parameter.
2. The method itself cannot change the parameter (so also the calling parameter) unintentionally.
3. The caller can itself define the transferred variable as const (I am loving using const where ever possible to avoid "careless mistakes").

The only disadvantage of const for me is: a word more has to be written :-) Same is also valid with transferred pointers instead of references for input parameters.

— Reply to this email directly, view it on GitHub https://github.com/JvanKatwijk/qt-dab/issues/289#issuecomment-1553733879, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCPHQCQIUQ3T34ZYBEG6A3XG2OBFANCNFSM6AAAAAAYAX5SQY . You are receiving this because you commented.Message ID: @.***>

-- Jan van Katwijk

[tomneda]

Sorry, the problem is still exiting on the head of master (https://github.com/JvanKatwijk/qt-dab/commit/ca66d5a911cfd8a88f7e357198a36432980957a0).

This needs to be changed (adding & to the parameter): In data-processor.h:47: void addtoFrame (std::vector<uint8_t> &); In data-processor.cpp:81 void dataProcessor::addtoFrame (std::vector<uint8_t> & outV) {

Regarding const, my argments were not due to compiler optimization but about readability, maintainability and reuse-ability.

1. readability: a const at a reference or pointer indicates the user clearly that the parameter can only be used as input parameter and no data were changed at caller side.
2. maintainability: the const inhibits that the data in the method can be changed unintentionally. Otherwise the data from the caller could be changed also. This could lead to a bigger debugging session.
3. reuse-ability: if an input parameter with reference or pointer is not set as const, the caller is forced to keep its variable also mutable, because it is not allowed to transfer a const "variable" as a referenced parameter without const. So it could be a bit embarrassing using this method in another code as it has to be analyzed and adapted first.

In my company we "must" use tools like clang-tidy or SonarQube. This tools track such things and they would reject a delivery (sometimes annoyingly) without this const. So, I got also sensitive about this :-)

[JvanKatwijk]

ad 1 You are right, will be changed ad 2 I am not with you there, I am not critizing your company but I am not in favour. I would have preferred a Ada type of parameterpassing with "in" "inout" and "out" not the kind of afterthoughts as appplied to C and C++

Op vr 19 mei 2023 om 20:42 schreef TomNeda @.***>:

Sorry, the problem is still exiting on the head of master (ca66d5a https://github.com/JvanKatwijk/qt-dab/commit/ca66d5a911cfd8a88f7e357198a36432980957a0 ).

This needs to be changed (adding & to the parameter): In data-processor.h:47: void addtoFrame (std::vector &); In data-processor.cpp:81 void dataProcessor::addtoFrame (std::vector & outV) {

Regarding const, my argments were not due to compiler optimization but about readability, maintainability and reuse-ability.

1. readability: a const at a reference or pointer indicates the user clearly that the parameter can only be used as input parameter and no data were changed at caller side.
2. maintainability: the const inhibits that the data in the method can be changed unintentionally. Otherwise the data from the caller could be changed also. This could lead to a bigger debugging session.
3. reuse-ability: if an input parameter with reference or pointer is not set as const, the caller is forced to keep its variable also mutable, because it is not allowed to transfer a const "variable" as a referenced parameter without const. So it could be a bit embarrassing using this method in another code as it has to be analyzed and adapted first.

In my company we "must" use tools like clang-tidy or SonarQube. This tools track such things and they would reject a delivery (sometimes annoyingly) without this const. So, I got also sensitive about this :-)

— Reply to this email directly, view it on GitHub https://github.com/JvanKatwijk/qt-dab/issues/289#issuecomment-1555085048, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCPHQEXT5CK7GM6I7LBZG3XG65J3ANCNFSM6AAAAAAYAX5SQY . You are receiving this because you commented.Message ID: @.***>

-- Jan van Katwijk

[tomneda]

now it works! :-) Thanks for that adaption.

Unfortunately, I know only very little about Ada :-(

For C and C++, concepts with pointers and references are language inherent and so there are concepts to avoid the drawbacks working with pointers (such using smart pointers or containers, and const).

For this missing keywords like "in", "inout" and "out" we also must consider this in the naming of the parameter variables like: my_method(const Type & iIamAInputVariable, Type & ioIamForInputAndOutput, Type & oIamForOutputOnly); So we must add "i", "io" or "o" in front of the parameter. In former times it was written "in", "inout" and "out". So, somehow a mitigation regarding Ada. :-)

I will close this "issue" now.