Issue617

Title Remove dependency on boost:any
Priority wish Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To florian Keywords
Optional summary

Created on 2016-01-05.18:44:15 by florian, last changed by florian.

Files
File name Uploaded Type Edit Remove
test_any.cc florian, 2016-01-06.19:40:12 text/x-c++src
Messages
msg5046 (view) Author: florian Date: 2016-01-07.19:42:49
Merged, pushed and fixed the build on Windows.
msg5045 (view) Author: jendrik Date: 2016-01-07.10:33:59
Jendrik is fine with that.
msg5044 (view) Author: florian Date: 2016-01-07.10:07:00
Thanks for all the tests and comments. I updated the pull request as you
suggested. I think we can remove ext/boost and merge this now, if Jendrik is
fine with implementing a non-boost bitset for issue600.
msg5043 (view) Author: malte Date: 2016-01-06.22:23:12
I also looked at (2). I think if we use const in the missing place, this one
goes away, too. This set of constructors (plus default constructor) passes all
the tests for me and doesn't need the type traits:

    Any(const Any &other)
        : content(other.content ? other.content->clone() : nullptr) {
    }

    template<typename ValueType>
    Any(const ValueType &value)
        : content(Utils::make_unique_ptr<Holder<ValueType>>(value)) {
    }
msg5042 (view) Author: malte Date: 2016-01-06.22:04:59
Any a_int_literal_ctor(42);

fails because the Any constructor from a value type requires a non-const reference.
We have a constructor for "int &", but not for "const int &", which is what we
need here.

If I add a const version of that constructor or make the existing one const, all
tests pass for me including the one above (but excluding the float-to-int one
mentioned in the previous message). I think we should make the existing one
const -- I see no reason why it should be non-const.
msg5041 (view) Author: malte Date: 2016-01-06.21:56:20
...and looking at the test file, I'd actually expect this int-to-float
conversion to fail. Is that line in case 6) there by accident?
msg5040 (view) Author: malte Date: 2016-01-06.21:50:59
Do the tests in the attached file work for you? I get an error in the 6th one:

$ g++ --version && g++ -Wall -Wno-unused-variable -std=c++11 test_any.cc -o
test_any && ./test_any
g++ (Ubuntu 4.9.2-10ubuntu13) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

1) Default ctor
2) Value ctor
42
42
hello world
hello world
3) Copy ctor
42
hello world
hello world
hello world
hello world
4) Assign Any
42
42
5) Assign value
42
6) Working cast
terminate called after throwing an instance of 'BadAnyCast'
  what():  BadAnyCast: failed conversion using any_cast
Aborted (core dumped)
msg5039 (view) Author: malte Date: 2016-01-06.21:24:38
As discussed offline, I'd like to understand better what is going on with (1)
before we proceed.

For (2) I wonder if a simpler solution would be to define an additional overload
with a non-const reference. Here are two related links:

http://stackoverflow.com/questions/16568291/function-template-overloading-const-vs-const
https://channel9.msdn.com/Series/C9-Lectures-Stephan-T-Lavavej-Core-C-/Stephan-T-Lavavej-Core-Cpp-3-of-n

(I didn't watch the video, but the description seems to match and the speaker is
a very well known C++ guru.)
msg5038 (view) Author: florian Date: 2016-01-06.19:40:12
I did some tests with the attached file and found two things:

1) "Any a(3);" does not work. Since we don't need it, I just added it to the
list of features that we do not support.

2) "Any a; Any copy(a);" called the templated constructor for values instead of
the copy constructor. The former expects a "ValueType&" for any ValueType ("Any"
in this case) while the latter expects a "const Any&" and is thus the worse
match. I tried to disable the constructor for ValueType = Any similar to the way
it is handled in boost, using enable_if and SFINAE
(https://de.wikipedia.org/wiki/Substitution_failure_is_not_an_error). It seems
to work fine now.
msg5033 (view) Author: malte Date: 2016-01-06.00:48:02
We should change the text because there is no accompanying LICENSE_1_0.txt (and
I don't think we should add one). Having read the license, I think a simple way
to comply is to copy the license text itself into the source file, e.g. at the
top or bottom. I wouldn't want to do this with something heavy like the GPL, but
this one is short and simple, and I think it makes sense to have the licensing
terms right there.

Given that you substantially modified the code, we shouldn't just include the
old copyright notice there without further explanations, but I also wouldn't
want to add a detailed copyright statement of our own because we would have to
keep updating that in the future. Instead, I'd suggest something like:

/*

This source file was derived from the boost::any library version XYZ by XYZ.
Original copyright statement and license for this original source follow.

... [include stuff here]

*/


If we can find time, we can go over the code briefly in person. (I don't have
time to comment on bitbucket, which takes longer than discussing things live.)
But it looks good in general, so if it works, feel free to merge without further
review. One thing I noticed is some places with "0" instead of "nullptr".
msg5032 (view) Author: florian Date: 2016-01-06.00:09:48
I created a pull request for an implementation I mostly took from boost::any.
I'm not sure if the copyright statement and license is fine the way it is: the
license says that it should be included, and the text in the file says that
there is an "accompanying file LICENSE_1_0.txt" which does not exist. (This was
the same way before the issue.) Should the copyright statement stay like this or
should we add our own?

https://bitbucket.org/flogo/downward-issues/pull-requests/12

I did not delete the boost headers yet because this makes the pull request too
large to display on bitbucket.
msg5029 (view) Author: malte Date: 2016-01-05.18:51:02
I think neither an `any` class nor a `dynamic_bitset` class for our purposes
would be particularly challenging, as we don't need to implement the full
interface of the boost classes. I'd estimate they can be implemented in one
afternoon. (Now apply the usual multipliers for such estimates. ;-))
msg5028 (view) Author: florian Date: 2016-01-05.18:44:15
We currently only have one dependency on boost: the option parser uses
boost::any. If we could implement our own version of "any", we could remove the
boost code from the repository.

I found a useful stackoverflow answer on that topic:
  http://stackoverflow.com/a/4989141/892961
The answer also links an article that explains the method behind the boost
implementation:
http://www.two-sdg.demon.co.uk/curbralan/papers/ValuedConversions.pdf
History
Date User Action Args
2016-01-07 19:42:49floriansetstatus: chatting -> resolved
assignedto: florian
messages: + msg5046
summary: This might make issue616 obsolete. TODO: remember to delete boost headers before merging ->
2016-01-07 10:33:59jendriksetmessages: + msg5045
2016-01-07 10:07:00floriansetmessages: + msg5044
2016-01-06 22:23:12maltesetmessages: + msg5043
2016-01-06 22:04:59maltesetmessages: + msg5042
2016-01-06 21:56:20maltesetmessages: + msg5041
2016-01-06 21:50:59maltesetmessages: + msg5040
2016-01-06 21:24:38maltesetmessages: + msg5039
2016-01-06 19:40:12floriansetfiles: + test_any.cc
messages: + msg5038
2016-01-06 00:48:02maltesetmessages: + msg5033
2016-01-06 00:09:48floriansetmessages: + msg5032
summary: This might make issue616 obsolete. -> This might make issue616 obsolete. TODO: remember to delete boost headers before merging
2016-01-05 18:51:02maltesetstatus: unread -> chatting
messages: + msg5029
2016-01-05 18:44:15floriancreate