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

QgsProcessingAlgorithm: add variants of addParameter() and addOutput() that accept a std::unique_ptr #59512

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Nov 20, 2024

If we had those 2 new variants and used them, we would have immediately detected the issue fixed by #59511, because the std::move() would have zeroed the smart pointer causing an immediate null-ptr derefrence when later used.

There is no good reason in 2024 to use explicit new and delete :-)

@github-actions github-actions bot added this to the 3.42.0 milestone Nov 20, 2024
Copy link

github-actions bot commented Nov 20, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit aff6837)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit aff6837)

@nyalldawson
Copy link
Collaborator

@rouault I've reached out to the PyQt list to see if we can get #59513 in shape. If so, then we could just switch the original methods over to the unique_ptr variants and avoid the double-API...


// backwards compatibility parameters
// TODO QGIS 4: remove compatibility parameters and their logic
QgsProcessingParameterField *orderField = new QgsProcessingParameterField( QStringLiteral( "ORDER_FIELD" ),
QObject::tr( "Order field" ), QVariant(), QString(), Qgis::ProcessingFieldParameterDataType::Any, false, true );
auto orderField = std::make_unique<QgsProcessingParameterField>( QStringLiteral( "ORDER_FIELD" ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto orderField = std::make_unique<QgsProcessingParameterField>( QStringLiteral( "ORDER_FIELD" ),
std::unique_ptr<QgsProcessingParameterField> orderField = std::make_unique<QgsProcessingParameterField>( QStringLiteral( "ORDER_FIELD" ),

I don't think we should use auto with these plain and not-that-long type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should use auto with these plain and not-that-long type.

oh please please, can't we make an exception here...? I dislike so much reading QGIS code because of all that type boilerplate that brings more noise than value. Here the scope of that variable is so reduced (3 lignes), so that àuto doesn't hurt. (in my wishlist I would like to kill all those painful explicit QStringLiteral() or QLatin1String() stuff that also make the code hard to read. In an ideal world, the compiler would figure that out itself)

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 we need to explicitly describe when we can use auto or not. I was thinking about this comment when I made mine.

But in the end, that's a different scenario. Just a quicklook on stackoverflow and it looks new/make_unique instanciation are also good place to use auto.

So ok then :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with auto being used as the type for a std::make_unique call, since it's still immediately clear what the type is when reviewing code.

(That's my big issue with auto -- it makes in-depth reviews impossible to do without first loading the code into an IDE.)

I suggest we need a QEP for this policy, and we could explicitly document where auto should be used vs not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

baby steps towards having a place to describe coding policies: qgis/QGIS-Enhancement-Proposals#310

Copy link

github-actions bot commented Dec 5, 2024

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 5, 2024
@nyalldawson
Copy link
Collaborator

@rouault given #59513 stalled on getting a working python -> unique_ptr conversion working, let's proceed with this one! Can you fix the conflicts?

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 9, 2024
@rouault rouault force-pushed the QgsProcessingAlgorithm_addParameter_unique_ptr branch from 5a1ebdb to aff6837 Compare December 9, 2024 01:34
@rouault
Copy link
Contributor Author

rouault commented Dec 9, 2024

Can you fix the conflicts?

done

@nyalldawson nyalldawson merged commit b87991a into qgis:master Dec 9, 2024
31 checks passed
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.

3 participants