Turn class templates into a member classes of class templates to further narrow scope of ADL #83

ericniebler · 2020-03-06T01:50:43Z

No description provided.

…tion-state types as non-template classmembers of a class template to avoid template parametersfrom becoming associated namespaces.

ericniebler · 2020-03-06T01:50:58Z

Not quite done yet, but getting there.

lewissbaker · 2020-03-06T18:35:38Z

include/unifex/allocate.hpp

+  template<>
+  struct _fn::_impl<false> {
+    template <typename Sender>
+    auto operator()(Sender&& predecessor) const


We seem to be inconsistent with the naming of the input sender.
We use 'sender', 'source' and 'predecessor' in various places.
Ideally we'd be consistent. Any preferences?

No strong feelings.

include/unifex/allocate.hpp

include/unifex/awaitable_sender.hpp

include/unifex/dematerialize.hpp

include/unifex/finally.hpp

include/unifex/when_all.hpp

include/unifex/scheduler_concepts.hpp

lewissbaker · 2020-03-06T21:47:33Z

include/unifex/thread_unsafe_event_loop.hpp

+      callback_.destruct();
+      if constexpr (is_stop_never_possible_v<
+                        stop_token_type_t<Receiver&>>) {
+        unifex::set_value(std::move(receiver_));


I just noticed that this call to set_value() could potentially be throwing (since we don't require that set_value() customisations are noexcept any more).

We probably need to do a pass through libunifex to pick up calls to set_value() and wrap them in try/catch if they are not noexcept.

I'll leave it for now.

include/unifex/timed_single_thread_context.hpp

…uence(). Make CPO and sender/receiver arguments deduced and then constrain to SFINAE out if the types don't match exactly. Also modify handling of sequence() overload for 3 or more parameters to avoid infinite recursion of function instantiation under msvc by adding an explicit third argument so that the overload would not be considered for the recrusive call to sequence() with two argments.

…nsform(). Make CPO and sender/receiver arguments deduced and then constrain to SFINAE out if the types don't match exactly.

…n_all(). Make CPO and sender/receiver arguments deduced and then constrain to SFINAE out if the types don't match exactly.

include/unifex/sender_concepts.hpp

…, Args...> on finally(), sequence() and when_all(). This allows examples/async_trace.cpp to build under msvc.

MSVC was too eagerly istantiating tag_invoke() overloads where there were non-deduced paramters (like a CPO or receiver parameter). Now make these deduced parameters and explicitly exclude these overloads using enable_if so that the examples/materialize builds under msvc.

…late function instantiation. Added UNIFEX_[DECLARE/USE]_NON_DEDUCED_TYPE(NAME, TYPE). On MSVC this lowers to enable_if-based SFINAE. On other compilers this lowers to a non-template argument. Updated the materialize/dematerialize algorithms to use these macros where appropriate.

…voke overloads

lewissbaker · 2020-03-12T19:43:35Z

include/unifex/any_unique.hpp

+  struct type {
+   private:
+      template <typename T>
+      static void* get_object_address(T&& t) {


Should this be noexcept?

lewissbaker · 2020-03-12T19:56:39Z

include/unifex/async_mutex.hpp

    };
+    template <typename Receiver>
+    using operation = typename _op<std::decay_t<Receiver>>::type;


We are inconsistent with use of decay_t vs remove_cvref_t when decaying the Receiver types.
Should always just use remove_cvref_t?

lewissbaker · 2020-03-12T20:14:51Z

include/unifex/blocking.hpp

+  template <bool>
+  struct _impl {
+    template <typename Sender>
+    constexpr blocking_kind operator()(const Sender&) noexcept {
+      return blocking_kind::maybe;
+    }
+  };


Should be private: ?

include/unifex/config.hpp.in

lewissbaker · 2020-03-12T21:15:50Z

include/unifex/materialize.hpp

+          typename Func,
          UNIFEX_DECLARE_NON_DEDUCED_TYPE(CPO, tag_t<visit_continuations>),
-          UNIFEX_DECLARE_NON_DEDUCED_TYPE(R, receiver),
-          typename Func>
+          UNIFEX_DECLARE_NON_DEDUCED_TYPE(R, receiver)>


Why was the change to put Func first needed?

Because clang was not allowing a non-defaulted template parameter to follow ones that had defaults.

That would be the case for classes, but I didn't think it was the case for functions where template parameters can be deduced.
eg. see https://godbolt.org/z/oQmQ_d which has leading defaulted templated args followed by non-defaulted-but-deduced template args.

lewissbaker · 2020-03-12T23:33:11Z

include/unifex/sender_awaitable.hpp

 auto operator co_await(Sender&& sender) {
-  return sender_awaiter<Sender, Result>{(Sender&&)sender};
+  return _coroutine::sender_awaiter<Sender, Result>{(Sender&&)sender};
 }


I wonder if moving the existing sender types out of the unifex namespace and into child namespaces will mean that this operator co_await() overload is not found for some operations...

Probably so, but that was always going to be the case. We expect others to define sender types in their namespace. It's one of the reasons why sender_base exists in the P0443 design.

lewissbaker · 2020-03-12T23:42:31Z

include/unifex/single.hpp

+  inline constexpr struct _fn {
+    template <typename Sender>
+    auto operator()(Sender&& sender) const {
+      return _single::stream<std::remove_cvref_t<Sender>>{(Sender&&)sender};


You are double-applying the std::remove_cvref_t both here and in the _single::stream alias.

lewissbaker · 2020-03-12T23:49:24Z

include/unifex/single_thread_context.hpp

@@ -37,5 +37,8 @@ class single_thread_context {
    return loop_.get_scheduler();
  }
 };
+} // namespace _single_thread
+
+using single_thread_context = _single_thread::context;


Do we need to ADL-isolate this class as it doesn't customise any ADL-calls?

Probably not strictly necessary given the other ADL mitigations we have in place, but defining types in ADL-blocking namespaces is a good habit, and I would rather we are consistent about it.

lewissbaker · 2020-03-12T23:57:15Z

include/unifex/stop_immediately.hpp

+          template callback_type<cancel_next_callback>>
+            stopCallback_;
+
+        using ST = stop_token_type_t<Receiver&>;


Can we move this type-alias further up so it can be used in the declaration of the stopCallback_ member?

lewissbaker · 2020-03-13T00:27:30Z

include/unifex/stop_immediately.hpp

+template<typename SourceStream, typename... Values>
+using stop_immediately_stream =
+    _stop_immediately::stream<SourceStream, Values...>;


Sometimes you've been putting the aliases for the stream classes inside the impl namespace (e.g. see single.hpp) and sometimes in the public unifex namespace (like here).

We should be consistent about whether the sender/stream types are public types or not (my preference is to keep them private for now)

In some places, the class name is the API (like never_stream). We could change this to be consistent and make never_stream a CPO that returns a _never::stream, but I think this diff is big enough.

EDIT: ... or never_stream could be an inline constexpr object of type _never::stream.

lewissbaker · 2020-03-13T05:27:58Z

We also need to update the io_uring_context and io_epoll_context types to apply ADL-isolation techniques to them as well.

The _inline symbol is a keyword under msvc.

Msvc wantscomplex constexpr expressions used as template arguments to be enclosed in parentheses.

lewissbaker

LGTM - we can address the remaining bits in future PRs.

Lewis Baker and others added 14 commits March 3, 2020 21:09

Modify just() to use better ADL insulation techniquesPut sender/opera…

087e34c

…tion-state types as non-template classmembers of a class template to avoid template parametersfrom becoming associated namespaces.

Modify transform() to use better ADL insulation techniques.

74e5ced

Modify finally() to use better ADL insuation techniques.

ce583d2

rework transform adl-isolation

e1bd2ba

turn transform into a customizeable algorithm

a972eca

adl-isolate allocate_[sender|operation]

efc6311

ADL-isolate awaitable_sender

ad2f0db

ADL-insulate dematerialize

e56186a

consistency fixes

2d602fc

ADL-isolate unifex::let

47bdaea

ADL-insulate unifex::materialize

0eb2f74

ADL insulate unifex::sequence

d530192

ADL-insulate unifex::via

e2a042e

ADL-insulate when_all

50fab33

ericniebler requested a review from lewissbaker March 6, 2020 01:50

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 6, 2020

Eric Niebler added 8 commits March 5, 2020 18:36

ADL-insulate unifex::retry_when

bb40421

avoid gcc warning

4293adb

Merge remote-tracking branch 'origin/master' into adl_isolation

d26067c

ADL-isolate unifex::reduce_stream

e343681

ADL-isolate indexed_for

b394049

ADL-isolate scheduler CPOs

dd1f492

ADL-isolate types in thread_unsafe_event_loop

af45c87

ADL-isolate types in timed_single_thread_context

31ea3a2

lewissbaker suggested changes Mar 6, 2020

View reviewed changes

lewissbaker reviewed Mar 6, 2020

View reviewed changes

include/unifex/timed_single_thread_context.hpp Outdated Show resolved Hide resolved

Eric Niebler added 3 commits March 6, 2020 14:25

ADL-isolate unifex::with_query_value

a7f4280

review feedback

6ae5224

Merge remote-tracking branch 'origin/master' into adl_isolation

0535c26

Lewis Baker added 4 commits March 9, 2020 22:54

ADL-isolate receiver types used by sync_wait() algorithm.

99d3b16

Work around msvc eagerness to instantiate tag_invoke overloads in tra…

0ee8593

…nsform(). Make CPO and sender/receiver arguments deduced and then constrain to SFINAE out if the types don't match exactly.

Work around msvc eagerness to instantiate tag_invoke overloads in whe…

ec18cb2

…n_all(). Make CPO and sender/receiver arguments deduced and then constrain to SFINAE out if the types don't match exactly.

ericniebler commented Mar 9, 2020

View reviewed changes

include/unifex/sender_concepts.hpp Show resolved Hide resolved

include/unifex/sender_concepts.hpp Show resolved Hide resolved

Lewis Baker and others added 14 commits March 9, 2020 23:51

Further constrain forwarding tag_invoke() to require is_invocable<CPO…

178c5a2

…, Args...> on finally(), sequence() and when_all(). This allows examples/async_trace.cpp to build under msvc.

Fix typo in when_all().

5a5a029

various tweaks to make clang happy and to disable ADL

968a089

more ADL-isolation

6352bcf

complex-ify the receiver CPOs to not add default tag_invoke overloads

fdbf2f1

complex-ify the sender and subschedule CPOs to not add default tag_in…

45e1bea

…voke overloads

more ADL isolation

35cfdd3

more ADL isolation

b307eff

more ADL isolation

0efaa86

more ADL isolation

e615084

bring type_erased_stream back from the dead. Add an example.

34dbdda

ADL-isolate type-erased stream

c773f75

lewissbaker suggested changes Mar 13, 2020

View reviewed changes

Lewis Baker and others added 5 commits March 13, 2020 15:26

Update manual_event_loop.cpp in sync with changes to header.

8d6006e

Fix syntax error in UNIFEX_DECLARE_NON_DEDUCED_TYPE macro on msvc.

6b87ce9

Rename_inline namespace to _inline_sched.

f789f85

The _inline symbol is a keyword under msvc.

Fix is_stop_never_possible_v under msvc.

1536f07

Msvc wantscomplex constexpr expressions used as template arguments to be enclosed in parentheses.

review feedback

195a864

lewissbaker approved these changes Mar 13, 2020

View reviewed changes

ericniebler merged commit dcc9f62 into master Mar 13, 2020

ericniebler deleted the adl_isolation branch March 13, 2020 18:54

lewissbaker mentioned this pull request Mar 13, 2020

Add support for building on Windows and MSVC #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn class templates into a member classes of class templates to further narrow scope of ADL #83

Turn class templates into a member classes of class templates to further narrow scope of ADL #83

ericniebler commented Mar 6, 2020

ericniebler commented Mar 6, 2020

lewissbaker Mar 6, 2020

ericniebler Mar 6, 2020

lewissbaker Mar 6, 2020

ericniebler Mar 6, 2020

lewissbaker Mar 12, 2020

lewissbaker Mar 12, 2020

lewissbaker Mar 12, 2020

lewissbaker Mar 12, 2020

ericniebler Mar 13, 2020

lewissbaker Mar 13, 2020

lewissbaker Mar 12, 2020

ericniebler Mar 13, 2020

lewissbaker Mar 12, 2020

lewissbaker Mar 12, 2020

ericniebler Mar 13, 2020

lewissbaker Mar 12, 2020

lewissbaker Mar 13, 2020

ericniebler Mar 13, 2020 •

edited

lewissbaker commented Mar 13, 2020

lewissbaker left a comment

Turn class templates into a member classes of class templates to further narrow scope of ADL #83

Turn class templates into a member classes of class templates to further narrow scope of ADL #83

Conversation

ericniebler commented Mar 6, 2020

ericniebler commented Mar 6, 2020

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

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

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

ericniebler Mar 13, 2020 • edited

Choose a reason for hiding this comment

lewissbaker commented Mar 13, 2020

lewissbaker left a comment

Choose a reason for hiding this comment

ericniebler Mar 13, 2020 •

edited