Skip to content

Commit

Permalink
Fix bug in v2 service Myth/GetBackendInfo
Browse files Browse the repository at this point in the history 
Calling this service was clearing out a global QString logPropagateArgs
so that the second time the service is called an empty string is
returned and after a call to this service any other code in the backend
that uses logPropagateArgs will not find the value.
  • Loading branch information
@bennettpeter
bennettpeter committed Mar 23, 2023
1 parent 023580f commit c92362e
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion mythtv/programs/mythbackend/servicesv2/v2myth.cpp
View file
Expand Up @@ -1113,7 +1113,10 @@ V2BackendInfo* V2Myth::GetBackendInfo( void )
pEnv->setUSER ( qEnvironmentVariable("USER",
qEnvironmentVariable("USERNAME")) );
pEnv->setMYTHCONFDIR ( qEnvironmentVariable("MYTHCONFDIR") );
pLog->setLogArgs ( logPropagateArgs );
// This needs to be first assigned to a new QString because setLogArgs
// uses a std_move which clears out the source variable while setting.
QString logArgs = logPropagateArgs;
pLog->setLogArgs ( logArgs );

// ----------------------------------------------------------------------
// Return the pointer... caller is responsible to delete it!!!
Expand Down

4 comments on commit c92362e

@kmdewaal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me that the std::move is the root cause here. I personally am not fond of using std::move to wring out the last bit of performance unless it is in a very time-critical inner loop and that does not seem to be the case here.

@bennettpeter
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether I should do away with the std::move altogether. It is used throughout the APIs and could cause problems any time a QString is used. I am not familiar with std::move and its purpose, but it seem dangerous. I think QStrings are optimized internally anyway.

@kmdewaal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a very good idea if you do not use std::move at all. It is effectively a pointer copy. The advantage of using C++ is that you do not need to worry about memory management and pointers etc anymore, thus reducing the possibility for errors. If you move pointers around then you are back to C-style programming with all the opportunities for making mistakes, like your bug does show.
When used at the right place then a std::move can avoid making a temporary copy of a class instance, as I understand it. This can be useful when writing templates etc but it should not be used in application code.
My understanding of QString is that they do internally reference counting, so there is no copying done anyway.
But even if it was doing a copy, I think it is better to make that copy than to start moving pointers around for the sake of efficiency.

@bennettpeter
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created issue #739

Please sign in to comment.