Skip to content

Fix bind const std::vector<boost::optional<>> with soci::use;#797

Closed
Lokimed wants to merge 3 commits intoSOCI:masterfrom
Lokimed:fix_optional_vector
Closed

Fix bind const std::vector<boost::optional<>> with soci::use;#797
Lokimed wants to merge 3 commits intoSOCI:masterfrom
Lokimed:fix_optional_vector

Conversation

@Lokimed
Copy link

@Lokimed Lokimed commented Mar 12, 2020

No description provided.

@Lokimed
Copy link
Author

Lokimed commented Mar 13, 2020

i will fix test. boost need test in common-tests.h

@Lokimed Lokimed force-pushed the fix_optional_vector branch from d8a227b to d7e418d Compare March 13, 2020 06:40
@vadz
Copy link
Member

vadz commented Mar 13, 2020

This isn't really specific to vectors of boost::optional, is it?

I'm not sure what's the purpose of convert_to_base(), it looks like some new calls to it were added by c166625 but then removed in 6da1388, however the function itself had existed before then and I think the calls to it from pre_use() are really necessary...

@Lokimed
Copy link
Author

Lokimed commented Mar 13, 2020

Without my fix not compile exchange(soci::use()) with any const vector<boost::optional<>>
But with const vector<> compile.
Maybe i still not understand all situation.
I neen more time for research.

@Lokimed
Copy link
Author

Lokimed commented Mar 13, 2020

problem is than for const vector<boost::optional> deduced user_type_tag instead basic_type_tag i think.
I try fix it.

@Lokimed
Copy link
Author

Lokimed commented Mar 13, 2020

I did It!!! Soon will be commit

@Lokimed Lokimed force-pushed the fix_optional_vector branch from d7e418d to 950e9e1 Compare March 13, 2020 22:18
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'm still not sure if this is the right thing to do... Any comments from the others would be very welcome.

sql << query, use(i, ind);

std::vector<int> numbers(100);
const std::vector<int>& cref_numbers = numbers;
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 really belongs here, wouldn't it be better to add this to the real tests, i.e. to common-tests.h?

Copy link
Author

Choose a reason for hiding this comment

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

This test not run every time? Why better move to common-tests.h?
If you insist, i will fix.


// test us bind with const vector<boost::optional>
{
const std::string query = "insert";
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit weird, what do the duplicated statements using this string instead of the literal constant supposed to check?

Copy link
Author

@Lokimed Lokimed Mar 14, 2020

Choose a reason for hiding this comment

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

I took so test from "soci/tests/empty/test-empty.cpp". I mean about duplicated statements.
I think aim is check char* and std::sting with operator << work correct and after possible bind values.
My aim is only check bind with vector<boost> with supported type and CONST vector<boost> with supported type.
If you want i can delete part of checks?

Comment on lines 155 to 163
#ifdef SOCI_HAVE_BOOST
template <typename T>
struct exchange_traits<boost::optional<T> >
{
typedef typename exchange_traits<T>::type_family type_family;
enum { x_type = exchange_traits<T>::x_type };
};
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Could this be done in include/soci/boost-optional.h? It seems like a more appropriate place for it.

Copy link
Author

Choose a reason for hiding this comment

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

I try do this

@Lokimed Lokimed force-pushed the fix_optional_vector branch 3 times, most recently from b06b1dd to 4c07118 Compare March 14, 2020 09:02
@Lokimed Lokimed changed the title try fix bind const std::vector<boost::optional<>>; Draft. Need more investigation. try fix bind const std::vector<boost::optional<>>; Mar 14, 2020
@Lokimed Lokimed force-pushed the fix_optional_vector branch from 4c07118 to d0afed1 Compare March 14, 2020 13:04
@Lokimed
Copy link
Author

Lokimed commented Mar 14, 2020

I watched on class

template <typename T> class use_type<std::vector<T> > : public vector_use_type

and also used const_cast

@Lokimed Lokimed force-pushed the fix_optional_vector branch from fea10ae to 7b6c954 Compare March 14, 2020 13:19
@Lokimed Lokimed changed the title Draft. Need more investigation. try fix bind const std::vector<boost::optional<>>; Fix bind const std::vector<boost::optional<>> with soci::use; Mar 14, 2020
@Lokimed
Copy link
Author

Lokimed commented Mar 14, 2020

Let's wath to class conversion_use_type

template <typename T> class conversion_use_type<std::vector<T> > : private base_vector_holder<T>, public use_type<std::vector<typename type_conversion<T>::base_type> >


class base_vector_holder have no method - convert_from_base


Let's watch to
class use_type

template <typename T> class use_type<std::vector<T> > : public vector_use_type

Have have no method - convert_from_base


Let's watch to
class vector_use_type

Have have no method - convert_from_base

but have method virtual void convert_to_base() {}


So if possible usw modern C++ and use override in all pace wheen possible, i think we must get compilation error for class conversion_use_type<std::vector
if mark his methos convert_from_base override.
But class conversion_use_type<std::vector is template. So ithink compiler not create method convert_from_base and we can delete this

@Lokimed Lokimed force-pushed the fix_optional_vector branch from 7b6c954 to 3a54d5d Compare March 14, 2020 13:55
@Lokimed
Copy link
Author

Lokimed commented Mar 27, 2020

Is there any problems which i need fix or explain?

@vadz
Copy link
Member

vadz commented Mar 27, 2020

Sorry, no, I just didn't have time to address this yet,

I don't think we still need changes in tests/empty/test-empty.cpp, but this is very minor and I can remove them myself when merging.

@Lokimed
Copy link
Author

Lokimed commented Mar 27, 2020

Sorry, no, I just didn't have time to address this yet,

I don't think we still need changes in tests/empty/test-empty.cpp, but this is very minor and I can remove them myself when merging.

Okay i will delete changes in tests/empty/test-empty.cpp soon.

@Lokimed Lokimed force-pushed the fix_optional_vector branch from 8c1d415 to bbd4c73 Compare March 27, 2020 14:16
@vadz
Copy link
Member

vadz commented Apr 4, 2020

Thanks, this really looks like the right thing to do to me, so even though I still don't fully understand what's going on here (and notably why were all these consts not there since the beginning), I'm going to merge it.

Thanks again!

@vadz vadz closed this in b32d480 Apr 4, 2020
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