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.

Sign up for GitHub

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

Jump to bottom

Allow passing rvalues to soci::use() #1102

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allow passing rvalues to soci::use() #1102

wants to merge 4 commits into from

Conversation

avpalienko
Copy link

@avpalienko avpalienko commented Nov 23, 2023 •
edited
Loading

It is a useful when we can write soci::use( 3 ) or soci::use (string_holder("once used string"))
So, for rvalue case we have to use a specific type of use_container. And the overload of the exchange() and do_use() functions. As a result we have a "use_type" object of new derived class "by_value_use_type" (or "by_value_conversion_use_type"), which contains the copy of the rvalue

@avpalienko
Copy link
Author

CI env problem:
Starting MySQL 5.7
Start-Service : Cannot find any service with service name 'MySQL57'.
At line:1 char:1

  • Start-Service 'MySQL57'
  •   + CategoryInfo          : ObjectNotFound: (MySQL[57](https://ci.appveyor.com/project/SOCI/soci/builds/48597466/job/r6lwn67f6ep8jnvb#L57):String) [Start-Service], ServiceCommandException
  + FullyQualifiedErrorId : NoServiceFoundForGivenName,Microsoft.PowerShell.Commands.StartServiceCommand

vadz
vadz reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

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

Sorry, I don't have time to really review the new code, but it seems clear that at least some comments explaining what it does would be helpful. Could you please try to add them?

TIA!

tests/oracle/test-oracle.cpp
@@ -180,39 +180,68 @@ struct basic_table_creator : public table_creator_base
}
};

TEST_CASE("Oracle nested statement", "[oracle][blob]")
TEST_CASE("Oracle nested statement", "[oracle][cursors]")
Copy link
Member

Choose a reason for hiding this comment

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

Is the change to this test case really related to the main topic of the PR?

Copy link
Author

Choose a reason for hiding this comment

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

No, this addition is an "incidental correction" )))

Copy link
Member

Choose a reason for hiding this comment

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

Can we leave it out and/or at least extract it into a separate commit (but a separate PR might be even better)?

tests/oracle/test-oracle.cpp Show resolved Hide resolved
@vadz
Copy link
Member

vadz commented Nov 23, 2023

CI env problem: Starting MySQL 5.7 Start-Service : Cannot find any service with service name 'MySQL57'. At line:1 char:1

* Start-Service 'MySQL57'

* ```
    + CategoryInfo          : ObjectNotFound: (MySQL[57](https://ci.appveyor.com/project/SOCI/soci/builds/48597466/job/r6lwn67f6ep8jnvb#L57):String) [Start-Service], ServiceCommandException
    + FullyQualifiedErrorId : NoServiceFoundForGivenName,Microsoft.PowerShell.Commands.StartServiceCommand
  ```

AppVeyor probably updated their MySQL version... Also not sure why did we suddenly start getting deprecation warnings in the GitHub CI builds. This is clearly unrelated to this PR, of course.

vadz
vadz reviewed Dec 11, 2023
View reviewed changes
Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

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

I'm sorry but I'm still unable to understand how does this code work. Maybe it seems obvious to you even without any comments, but this is not at all the case for me. Please consider adding some comments (and updating existing ones which are out of date now) to help me -- and the other people reading this code -- understand it.

TIA!

tests/oracle/test-oracle.cpp
@@ -180,39 +180,68 @@ struct basic_table_creator : public table_creator_base
}
};

TEST_CASE("Oracle nested statement", "[oracle][blob]")
TEST_CASE("Oracle nested statement", "[oracle][cursors]")
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave it out and/or at least extract it into a separate commit (but a separate PR might be even better)?

include/soci/use.h
@@ -48,10 +48,16 @@ struct use_container<T, no_indicator>

// soci::use is deleted for rvalues because it will likely lead to subtle stack-use-after-scope bugs.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not true any more and really should be updated, it's just too confusing otherwise.

include/soci/type-conversion.h
Comment on lines +186 to +187
template <typename T>
class by_value_conversion_use_type : value_holder<T>, public conversion_use_type<T>
Copy link
Member

Choose a reason for hiding this comment

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

It would be really great to have at least some explanation about what this class is used for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadz vadz vadz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@avpalienko @vadz