Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QJSValueList: Minor optimization #4767

Merged
merged 2 commits into from
May 24, 2022
Merged

QJSValueList: Minor optimization #4767

merged 2 commits into from
May 24, 2022

Conversation

uklotzde
Copy link
Contributor

A minor optimization and simplification, spotted while reviewing #4766.

@@ -25,7 +25,7 @@ class ControllerScriptEngineBase : public QObject {

virtual bool initialize();

bool executeFunction(QJSValue functionObject, const QJSValueList& arguments);
bool executeFunction(QJSValue functionObject, const QJSValueList& arguments = {});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this has performance implications. Can you ensure this won't cause any problems because of the construction of a temporary or something? The QList default constructor does not give any guarantees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't matter. Instead of creating default arguments in various places a single default parameter suffices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default instance would be created anyway, either here or at the call site.

Copy link
Member

Choose a reason for hiding this comment

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

The default instance would be created anyway, either here or at the call site.

My understanding was that the instance would've been created unconditionally, even when a different Qlist was supplied. So while it doesn't matter in the cases where the default arg was created at the call site, I think it does in the cases where we create non-default QList and pass that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, then your understanding is wrong. The substitution happens at compile time, either or.

But it wouldn't matter anyway, because the default constructor of all container types does almost nothing. QList is no exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is only syntactic sugar and for convenience, no performance implications.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks for the clarification.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM Thank you

@Swiftb0y Swiftb0y merged commit 7672cf1 into mixxxdj:main May 24, 2022
@uklotzde uklotzde deleted the qjsvaluelist branch July 3, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants