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

Is it better to use T{} instead of T() in Statement::getColumns()? #130

Closed
cycycyc opened this issue Jul 14, 2017 · 3 comments

Comments

@cycycyc
Copy link

cycycyc commented Jul 14, 2017

In the helper function called by getColumns<typename T, int N>, the returned T is constructed by T():

    // Helper function called by getColums<typename T, int N>
    template<typename T, const int... Is>
    T Statement::getColumns(const std::integer_sequence<int, Is...>)
    {
        return T(Column(mStmtPtr, Is)...);
    }

If I pass a struct without a constructor like:

    struct Test { int a; double b; const char* c; };
    auto test = query.getColumns<Test, 3>();

There will be a error:
error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'Statement_getColumns_Test::TestBody::Test'
I believe the initializer list, that is, the expanded parameter pack, will be first cast to Test, and then the move constructor will be invoked.

If I change T() to T{} like:

    // Helper function called by getColums<typename T, int N>
    template<typename T, const int... Is>
    T Statement::getColumns(const std::integer_sequence<int, Is...>)
    {
        return T{Column(mStmtPtr, Is)...};
    }

The error will be gone. The initializer list will be directly passed to construct the struct Test.

So I think it would be easier to use without defining a constructor for each struct when using getColumns(), by changing T() to T{}.

Please correct me if I'm wrong. Thanks.

@SRombauts

This comment has been minimized.

Copy link
Owner

SRombauts commented Jul 14, 2017

Hi,

Yes this would be helpful to use with structures without a constructor.
But the current design is to be used with classes with a constructor, which is also very useful!

So perhaps can you make a second template with a slightly different name instead of modifying the current one?

@cycycyc

This comment has been minimized.

Copy link
Author

cycycyc commented Jul 14, 2017

Thanks for your reply!

Yes, now it is useful, and changing to T{} can make it even more useful.
This change would not affect the original behavior on invoking the constructors.
Constructors can be invoked both by parentheses T() and by braces T{}.

I managed to pass the tests after I did the change, and you can double check.

@SRombauts SRombauts closed this in 078941c Jul 17, 2017
@SRombauts

This comment has been minimized.

Copy link
Owner

SRombauts commented Jul 17, 2017

Hey @cycycyc, thanks you very much for providing this improvement and your explanations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.