QEP 314: Relax prohibition against "auto"

[nyalldawson]

Allow use of auto for variable types when the variable type is explicitly stated during its initialization.

Specifically:

auto may be used for variable types if the type is explicit during variable initialization. Eg

// allowed, as the QgsPoint type is explicit during initialization:
auto pointObject = QgsPoint( 3, 4 );
// allowed, as the std::unique_ptr< QgsPoint >, std::shared_ptr< QgsPoint > types are explicit during initialization:
auto pointUniquePointer = std::make_unique< QgsPoint >( 3, 4 );
auto pointSharedPointer = std::make_shared< QgsPoint >( 3, 4 );

// NOT allowed, the argument types for the std::tuple are not explicit:
auto myTuple = std::make_tuple( 0, 5 );

[nyalldawson]

FYI @rouault

[m-kuhn]

What is the motivation for this?
While it's less typing for the author it's more brainwork for readers.

[wonder-sk]

Is there any reason we would want to write this instead of QgsPoint pointObject( 3, 4 ); ? The notation with auto is longer, and may involve extra copy of the temporary object?

[nyalldawson]

@wonder-sk

Is there any reason we would want to write this instead of QgsPoint pointObject( 3, 4 ); ? The notation with auto is longer, and may involve extra copy of the temporary object?

That's a good point. Should we reword this change to only allow for some explicit types like std::unique_ptr / std::shared_ptr ?

[wonder-sk]

Should we reword this change to only allow for some explicit types like std::unique_ptr / std::shared_ptr ?

yeah I think that would be good...

[nyalldawson]

@m-kuhn

What is the motivation for this?
While it's less typing for the author it's more brainwork for readers.

It's to make super verbose lines like these less painful to write:

std::unique_ptr< QgsProcessingParameter > extentParam = std::make_unique< QgsProcessingParameter >( .... ); 

in this case it's still immediately clear to reviewers/readers if it's written like:

auto extentParam = std::make_unique< QgsProcessingParameter >( .... ); 

[nirvn]

Being allergic to auto, +1 to restricting this relaxation to only std::unique_ptr / std::shared_ptr as suggested above.

[elpaso]

Being an auto worshiper, +1 to relaxing.

[nirvn]

@elpaso , heh, that had my laugh out loud :)

[troopa81]

not a great fan of auto so +1 to restricting to only std::unique_ptr / std::shared_ptr.

[dvdkon]

Now I feel like I have to voice my opinion too :) I like auto and would prefer allowing it instead of any non-primitive types, especially when the rough type is clear from context.
For example, I feel that QgsPointCloudIndex index = mLayer->index(); can be shortened to auto index = mLayer->index(); without any impact to readability.

By the way, thanks @nyalldawson for these coding style QEPs. A comprehensive coding standard was something I sorely missed when starting with QGIS development.

[nyalldawson]

@dvdkon

For example, I feel that QgsPointCloudIndex index = mLayer->index(); can be shortened to auto index = mLayer->index(); without any impact to readability.

My issue with that is that it requires an IDE to actually determine the object type. Eg in this case "index()" could quite validly return a QgsSpatialIndex, a QgsKdBushSpatialIndex, or something else entirely (a QHash? A QMap?). In short, it makes pull request reviews orders of magnitude more difficult.

[m-kuhn]

+1 for non trivial sounds good. Are iterators listed as acceptable somewhere already?

[dvdkon]

Small note: If auto is going to be restricted to some exhaustive list of cases, I think initialisation with new should also be included.
It's not nice or modern, but it's still used in many parts of the codebase.

[m-kuhn]

Small note: If auto is going to be restricted to some exhaustive list of cases, I think initialisation with new should also be included. It's not nice or modern, but it's still used in many parts of the codebase

I think this is worth discussing in a different proposal (to keep this one focussed), there are a few considerations to be made but in general it's a desirable direction IMHO.

[rouault]

I think initialisation with new should also be included.

generally speaking, we should avoid using new as much as possible and favor smart pointers (std::make_unique, etc) where possible

[dvdkon]

generally speaking, we should avoid using new as much as possible and favor smart pointers (std::make_unique, etc) where possible

I definitely agree, but it's used in the codebase today and it's not always possible to sanely replace it with a smart pointer.

In the future, I'd love to see some discussion on how we can further move away from raw pointers (the main blockers right now are Qt and SIP AFAIK), but for now I just don't want to have to keep writing LongClassName *longObjectName = new LongClassName;.

[nyalldawson]

I think this is worth discussing in a different proposal (to keep this one focused)

+1.

@dvdkon would you like to create that QEP?

[nyalldawson]

Ok, based on feedback this is now restricted to std::unique_ptr and std::shared_ptr only.