Remove boost refs #108

g-217 · 2024-01-24T19:33:04Z

No description provided.

thinlang · 2024-01-24T19:40:55Z

test/property_model_eval/adam_test_parser.cpp

@@ -107,7 +105,7 @@ adam_test_parser::adam_test_parser(std::istream& in_stream, const line_position_
 // namespace {

 // inline name_t qs_name(const queryable_sheet_t* qs) { return qs->name(); }
-//         adobe::find(sheets_m, boost::bind(&qs_name, _1) == sheet_name);
+//         adobe::find(sheets_m, std::bind(&qs_name, _1) == sheet_name);


could probably just delete this commented code?

fosterbrereton · 2024-01-24T21:11:52Z

adobe/table_index.hpp

@@ -1069,7 +1069,7 @@ inline typename table_index<Key, T, Compare, Transform>::iterator
 table_index<Key, T, Compare, Transform>::lower_bound(const key_type& key) {
    return adobe::lower_bound(
        index_m, key, compare_m,
-        boost::bind(transform_m, bind(indirect<value_type>(), boost::placeholders::_1)));
+        std::bind(transform_m, bind(indirect<value_type>(), std::placeholders::_1)));


Did you consider turning these into lambdas? I see you did that elsewhere in this PR.

fosterbrereton · 2024-01-24T21:12:47Z

source/adam.cpp

@@ -721,8 +718,8 @@ void sheet_t::implementation_t::add_output(name_t name, const line_position_t& p
                                           const array_t& expression) {
    // REVISIT (sparent) : Non-transactional on failure.
    cell_set_m.push_back(cell_t(access_output, name,
-                                boost::bind(&implementation_t::calculate_expression,
-                                            boost::ref(*this), position, expression),
+                                std::bind(&implementation_t::calculate_expression,


Same question here re: defining a lambda instead of using std::bind.

fosterbrereton · 2024-01-24T21:13:11Z

source/adam_evaluate.cpp

-    suite.add_interface_proc_m =
-        boost::bind(&adobe::sheet_t::add_interface, boost::ref(sheet), _1, _2, _3, _4, _5, _6);
-
+    suite.add_cell_proc_m = std::bind(&add_cell, std::ref(sheet), _1, _2, _3, _4);


Same question here re: bind -> lambda.

fosterbrereton · 2024-01-24T21:14:06Z

source/adam_parser.cpp

@@ -86,15 +83,15 @@ namespace adobe {

 adam_parser::adam_parser(std::istream& in, const line_position_t& position)
    : expression_parser(in, position) {
-    set_keyword_extension_lookup(boost::bind(&keyword_lookup, _1));
+    set_keyword_extension_lookup(std::bind(&keyword_lookup, _1));


Are these still necessary? Will this line compile without the bind?

(And so throughout this file.)

fosterbrereton · 2024-01-24T21:14:28Z

source/eve_evaluate.cpp

-    suite.add_interface_proc_m =
-        boost::bind(&adobe::sheet_t::add_interface, boost::ref(sheet), _1, _2, _3, _4, _5, _6);
-    suite.finalize_sheet_proc_m = boost::bind(&sheet_t::update, boost::ref(sheet));
+    suite.add_view_proc_m = std::bind(


Consider bind -> lambda?

fosterbrereton

In general I am in favor of this change - thank you for the contribution! My main questions are around the use of std::bind, and instead migrating the code that uses it to lambdas instead. Lambdas are much easier to read than std::bind expressions, so I am wondering if it would be valuable to transcribe some uses of bind to lambdas instead.

I will have to defer final approval of these changes to @sean-parent and the other stlab maintainers. But again, thank you for your help in removing these dependencies in favor of Standard alternatives.

sean-parent · 2024-01-25T06:17:44Z

adobe/json_helper.hpp

@@ -9,7 +9,6 @@
 #define ADOBE_JSON_HELPER_HPP

 #include <adobe/any_regular.hpp>
-#include <adobe/array.hpp>


Why are you removing adobe/array.hpp when adobe::array_t is used in this file?

sean-parent · 2024-01-25T06:32:07Z

test/unit_tests/forest/forest_test.cpp

-    boost::function_requires<boost::BidirectionalIteratorConcept<forest<int>::const_iterator>>();
-    boost::function_requires<boost::BidirectionalIteratorConcept<forest<int>::preorder_iterator>>();
-    boost::function_requires<
+    std::function_requires<boost::BidirectionalIteratorConcept<forest<int>::iterator>>();


boost::function_requires is part of boost concept checks, there is no direct std:: equivalent. This code will not compile.

sean-parent

There are a number of places where boost headers are removed and the equivalent standard headers are not added. A couple of places where an adobe/ header is removed even though the declarations within are used in the file. boost::function_requires was renamed even though there is no direct equivalent in the standard (some of this could be replaced with static asserts and standard type traits if you feel so inclined).

For the bind expressions - the transformation to a lambda, would be nice, but is also error prone. The one place a saw it you mapped a cref to a & capture, which is not the same (cref's can be copied and express immutability). With just a quick review of the change it is unclear if this is a correct transformation. Unless you are up for a careful analysis, I would recommend just mapping boost::bind to std::bind for now.

The code does not pass CI on any platform and it is clear the unit tests have not been run locally before submitting the PR.

Please make sure the changes build and run on at least one platform before submitting a PR.

sean-parent · 2024-01-25T20:09:54Z

There are a number of places where boost headers are removed, and the equivalent standard headers are not added.

I saw the additional commit - (at least) this issue hasn't been addressed.

g-217 · 2024-01-29T12:07:01Z

source/adam_evaluate.cpp

+                                         const array_t& initializer, const line_position_t& position2,
+                                         const array_t& expression, const std::string& brief,
+                                         const std::string& detailed) -> void {
+        sheet.add_interface(name, linked, position1, initializer, position2, expression);


There is a mismatch between signatures of add_interface_proc_m and sheet_t::add_interface. While former has 8 args, later has only 6.

using add_interface_proc_t = std::function<void( name_t cell_name, bool linked, const line_position_t& position1, const array_t& initializer, const line_position_t& position2, const array_t& expression, const std::string& brief, const std::string& detailed)>;

void sheet_t::add_interface(name_t name, bool linked, const line_position_t& position1, const array_t& initializer, const line_position_t& position2, const array_t& expression)

I believe boost bind just drops the extra arguments. Ifstd::bind behaves differently, I'm fine with the lambda here (the two missing arguments are for comments from the parser but the sheet doesn't do anything with them.

sean-parent · 2024-01-30T00:17:17Z

adobe/placeable_concept.hpp

@@ -70,8 +70,8 @@ void place(T& t, const place_data_t& place_data) { t.place(place_data); }
 \ingroup placeable_concept
 */
 template <PlaceableConcept T>
-concept_map PlaceableConcept<boost::reference_wrapper<T>>{
-    void measure(boost::reference_wrapper<T> & r,
+concept_map PlaceableConcept<std::reference_wrapper<T>>{


Include the appropriate header.

sean-parent · 2024-01-30T01:45:18Z

There are two minor comments on the code (one missing header and one comment on bind->lambda). I'm not sure why CI is failing.

…nges

g-217 · 2024-01-30T11:45:03Z

test/property_model_eval/queryable_sheet.cpp

    }

    for (queryable_sheet_t::index_t::iterator iter = output_index_m.begin(),
                                              e = output_index_m.end();
         iter != e; ++iter) {
        std::size_t i(iter->second);
-        sheet_m.monitor_value(name_m[i], boost::bind(adobe::assign(), _1, boost::ref(value_m[i])));
+        sheet_m.monitor_value(name_m[i], [&value_m_at_index_i = value_m[i]](any_regular_t val) {


With clang-14/16, std::bind is not working with type any_regular_t for adobe::assign.

Do we have an understanding of why? Maybe it would be better to fix the underlying cause.

sean-parent · 2024-01-30T23:02:35Z

source/adam.cpp

@@ -38,7 +38,7 @@
 /**************************************************************************************************/

 using namespace std;
-using namespace std::placeholders;
+namespace ph = std::placeholders;


This is ugly. What is the ambiguity? Before accepting this change, I'd like to understand the issue.

A quick test - https://godbolt.org/z/Ydjff99Mo
Where does the ambiguity originate?

Error message is as following, boost/bind.hpp is indirectly included through boost/signals2/signal.hpp.

/root/expr/adobe_source_libraries/source/adam.cpp: In member function ‘adobe::sheet_t::connection_t adobe::sheet_t::implementation_t::monitor_enabled(adobe::name_t, const adobe::name_t*, const adobe::name_t*, const monitor_enabled_t&)’: /root/expr/adobe_source_libraries/source/adam.cpp:939:92: error: reference to ‘_1’ is ambiguous 939 | touch_set, iter->cell_set_pos_m, monitor, _1, _2)); | ^~ In file included from /root/expr/adobe_source_libraries/source/../adobe/adam.hpp:13, from /root/expr/adobe_source_libraries/source/adam.cpp:8: /usr/include/c++/9/functional:211:34: note: candidates are: ‘const std::_Placeholder<1> std::placeholders::_1’ 211 | extern const _Placeholder<1> _1; | ^~ In file included from /usr/include/boost/bind/bind.hpp:2356, from /usr/include/boost/bind.hpp:22, from /usr/include/boost/signals2/slot.hpp:15, from /usr/include/boost/signals2/connection.hpp:24, from /usr/include/boost/signals2/signal.hpp:22, from /root/expr/adobe_source_libraries/source/../adobe/adam.hpp:16, from /root/expr/adobe_source_libraries/source/adam.cpp:8: /usr/include/boost/bind/placeholders.hpp:46:38: note: ‘constexpr const boost::arg<1> boost::placeholders::_1’ 46 | BOOST_STATIC_CONSTEXPR boost::arg<1> _1;

sean-parent · 2024-01-30T23:12:51Z

Checking the build. I made a couple of comments that I'd like to understand better. Regarding adobe::assign. I think it would like work if we fixed assign:

//  `assign` is a function object type for the assignment operator.
// `assign{}(x, r)` is equivalent to `(void)(r = x)`
struct assign {
    typedef void result_type;

    template <class T, class U>
    void operator()(T&& x, U& r) {
        r = std::forward<T>(x);
    }
};

g-217 · 2024-01-31T19:16:51Z

Getting same error with new implementation of adobe::assign:

/usr/bin/clang++-16 -DADOBE_BUILT_WITH_CMAKE -DADOBE_STD_SERIALIZATION -DADOBE_STD_THREAD_LOCAL -DBOOST_ALL_NO_LIB -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_RANGE_ENABLE_CONCEPT_ASSERT=0 -DBOOST_SYSTEM_DYN_LINK -I/root/expr/adobe_source_libraries/source/.. -I/root/expr/double-conversion -I/root/expr/adobe_source_libraries/../double-conversion -g -std=c++17   -Wall -Wno-unused-variable -Wno-unknown-pragmas -Wno-uninitialized -Wno-return-type -Wno-unused-function -Wno-unused-local-typedefs -Wno-unused-parameter -Wno-unused-comparison -fconstexpr-depth=1024 -ftemplate-depth=1024 -MD -MT test/property_model_eval/CMakeFiles/property_model_eval.dir/queryable_sheet.cpp.o -MF test/property_model_eval/CMakeFiles/property_model_eval.dir/queryable_sheet.cpp.o.d -o test/property_model_eval/CMakeFiles/property_model_eval.dir/queryable_sheet.cpp.o -c /root/expr/adobe_source_libraries/test/property_model_eval/queryable_sheet.cpp
In file included from /root/expr/adobe_source_libraries/test/property_model_eval/queryable_sheet.cpp:8:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/functional:58:
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:308:46: fatal error: recursive template instantiation exceeded maximum depth of 1024
      template<typename _Up, typename _Up2 = __remove_cvref_t<_Up>>
                                             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:318:41: note: in instantiation of default argument for '__not_same<const std::_Head_base<1, std::reference_wrapper<adobe::version_1::any_regular_t>, false> &>' required here
      template<typename _Up, typename = __not_same<_Up>, typename
                                        ^~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:321:2: note: in instantiation of default argument for 'reference_wrapper<const std::_Head_base<1, std::reference_wrapper<adobe::version_1::any_regular_t>, false> &>' required here
        reference_wrapper(_Up&& __uref)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:319:40: note: while substituting deduced template arguments into function template 'reference_wrapper' [with _Up = const std::_Head_base<1, std::reference_wrapper<adobe::version_1::any_regular_t>, false> &, $1 = (no value), $2 = (no value)]
                = decltype(reference_wrapper::_S_fun(std::declval<_Up>()))>
                                                     ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:321:2: note: in instantiation of default argument for 'reference_wrapper<const std::_Head_base<1, std::reference_wrapper<adobe::version_1::any_regular_t>, false> &, __not_same<const _Head_base<1, reference_wrapper<any_regular_t>, false> &>>' required here
        reference_wrapper(_Up&& __uref)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:319:40: note: while substituting deduced template arguments into function template 'reference_wrapper' [with _Up = const std::_Head_base<1, std::reference_wrapper<adobe::version_1::any_regular_t>, false> &, $1 = (no value), $2 = (no value)]
                = decltype(reference_wrapper::_S_fun(std::declval<_Up>()))>
                                                     ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/refwrap.h:321:2: note: (skipping 1019 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
        reference_wrapper(_Up&& __uref)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:161:10: note: in defaulted copy constructor for 'std::_Bind<adobe::assign (std::_Placeholder<1>, std::reference_wrapper<adobe::version_1::any_regular_t>)>' first required here
            new _Functor(*__source._M_access<const _Functor*>());
                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:196:8: note: in instantiation of member function 'std::_Function_base::_Base_manager<std::_Bind<adobe::assign (std::_Placeholder<1>, std::reference_wrapper<adobe::version_1::any_regular_t>)>>::_M_clone' requested here
              _M_clone(__dest, __source, _Local_storage());
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:283:13: note: in instantiation of member function 'std::_Function_base::_Base_manager<std::_Bind<adobe::assign (std::_Placeholder<1>, std::reference_wrapper<adobe::version_1::any_regular_t>)>>::_M_manager' requested here
            _Base::_M_manager(__dest, __source, __op);
                   ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:611:33: note: in instantiation of member function 'std::_Function_handler<void (const adobe::version_1::any_regular_t &), std::_Bind<adobe::assign (std::_Placeholder<1>, std::reference_wrapper<adobe::version_1::any_regular_t>)>>::_M_manager' requested here
            _M_manager = &_My_handler::_M_manager;
                                       ^
/root/expr/adobe_source_libraries/test/property_model_eval/queryable_sheet.cpp:54:37: note: in instantiation of function template specialization 'std::function<void (const adobe::version_1::any_regular_t &)>::function<std::_Bind<adobe::assign (std::_Placeholder<1>, std::reference_wrapper<adobe::version_1::any_regular_t>)>, void, void>' requested here
        sheet_m.monitor_value(name, std::bind(adobe::assign(), ph::_1, std::ref(value_m[i])));

sean-parent · 2024-02-01T05:15:05Z

Getting same error with new implementation of adobe::assign:

I set up my own branch (and opened another PR for CI) to play with this. My first theory is it was caused by how the assignment is declared in any_regular_t, but that is not the case. The other possibility that I'll explore tomorrow is this is caused because you can construct an any_regular_t from a reference_wrapper - but I don't see how that would cause recursion.

sean-parent · 2024-02-01T23:28:49Z

I will close this PR - the code was moved to PR109 so I can play with solving the ref(any_regular_t) issue with clang 14 (I was unsuccessful - and reverted to using lambda with comments). Thanks @g-217!

Remove boost refs

f3e4781

thinlang reviewed Jan 24, 2024

View reviewed changes

fosterbrereton reviewed Jan 24, 2024

View reviewed changes

sean-parent reviewed Jan 25, 2024

View reviewed changes

sean-parent requested changes Jan 25, 2024

View reviewed changes

Revering unnecessary changes

ccb55c3

Addressing review comments

8401115

g-217 commented Jan 29, 2024

View reviewed changes

Compilation fix

8a8e1db

g-217 requested a review from sean-parent January 29, 2024 18:55

sean-parent reviewed Jan 30, 2024

View reviewed changes

Fixing compilation failures on my machine; These are good to have cha…

09d462a

…nges

g-217 commented Jan 30, 2024

View reviewed changes

g-217 requested a review from sean-parent January 30, 2024 17:32

sean-parent reviewed Jan 30, 2024

View reviewed changes

sean-parent closed this Feb 1, 2024

g-217 deleted the gautam/asl-refact branch February 2, 2024 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove boost refs #108

Remove boost refs #108

g-217 commented Jan 24, 2024

thinlang Jan 24, 2024

fosterbrereton Jan 24, 2024

fosterbrereton Jan 24, 2024

fosterbrereton Jan 24, 2024

fosterbrereton Jan 24, 2024

fosterbrereton Jan 24, 2024

fosterbrereton left a comment

sean-parent Jan 25, 2024

sean-parent Jan 25, 2024

sean-parent left a comment

sean-parent commented Jan 25, 2024

g-217 Jan 29, 2024

sean-parent Jan 30, 2024

sean-parent Jan 30, 2024

sean-parent commented Jan 30, 2024

g-217 Jan 30, 2024 •

edited

Loading

sean-parent Jan 30, 2024

sean-parent Jan 30, 2024

sean-parent Jan 31, 2024

g-217 Jan 31, 2024

sean-parent commented Jan 30, 2024

g-217 commented Jan 31, 2024

sean-parent commented Feb 1, 2024

sean-parent commented Feb 1, 2024

Remove boost refs #108

Remove boost refs #108

Conversation

g-217 commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fosterbrereton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-parent left a comment

Choose a reason for hiding this comment

sean-parent commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-parent commented Jan 30, 2024

g-217 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-parent commented Jan 30, 2024

g-217 commented Jan 31, 2024

sean-parent commented Feb 1, 2024

sean-parent commented Feb 1, 2024

g-217 Jan 30, 2024 •

edited

Loading