Commit 68cb50c5 authored by Alex Wang's avatar Alex Wang Committed by Facebook Github Bot

Remove std:: from index_sequence in Synchronized ctor

Summary:
Apple Clang 9 (clang-900.0.39.2) on macOS 10.13.3, when configured with
`FOLLY_USE_LIBCPP`, fails to build `SynchronizedTest.cpp` with the following
error:

    In file included from /Users/awang/code/folly/folly/test/SynchronizedTest.cpp:20:
    /Users/awang/code/folly/folly/Synchronized.h:511:9: error: no matching constructor for
          initialization of 'folly::Synchronized<int, NonDefaultConstructibleMutex>'
          : Synchronized{std::piecewise_construct,
            ^           ~~~~~~~~~~~~~~~~~~~~~~~~~~
    /Users/awang/code/folly/folly/test/SynchronizedTest.cpp:539:25: note: in instantiation
          of function template specialization 'folly::Synchronized<int,
          NonDefaultConstructibleMutex>::Synchronized<int &&, int &&>' requested here
      auto&& synchronized = folly::Synchronized<int, NonDefaultConstructibleMutex>{
                            ^
    /Users/awang/code/folly/folly/Synchronized.h:499:12: note: candidate constructor not
          viable: no known conversion from 'const std::__1::piecewise_construct_t' to
          'in_place_t' (aka 'folly::in_place_tag (&)(folly::in_place_tag)') for 1st argument
      explicit Synchronized(in_place_t, Args&&... args)
               ^
    /Users/awang/code/folly/folly/Synchronized.h:764:3: note: candidate template ignored:
          could not match 'std::__1::integer_sequence' against 'folly::integer_sequence'
      Synchronized(
      ^
    <other constructors not viable due to number-of-parameter mismatches>

The proximate cause of the error is that `make_index_sequence` is
generating a `folly::index_sequence`, but the delegatee constructor only
accepts `std::index_sequence`.

Changing the delegatee constructor to take an unqualified `index_sequence`
solves the problem, since the return type for an unqualified
`make_index_sequence` should be the same as the unqualified
`index_sequence`.

This appears to have been introduced in
8581e9e0.

Clang ends up using the Folly implementation of
`make_integer_sequence` because `__cpp_lib_integer_sequence` is
still not defined, even though `integer_sequence` support was added
to libc++ way back in mid-2013, in revision 185343. There is a bug open
 on the LLVM bug tracker (#18584 [1]) to add the
`__cpp_lib_integer_sequence` and other feature test macros,
but the last activity on it was in late 2014. This results in
the compiler successfully finding the definition for `std::index_sequence`
(unlike what would happen with C++11 compilers), but using the folly
implementation for `make_index_sequence`.

Qualified `std::index_sequence` is used in a few other places, but `std::`
was not removed from those usages because they reside in functions that
have C++14 features that would prevent a C++11 compiler from compiling
them at all, let alone with a `std::` v.s. `folly::`
mismatch:

    - folly/test/FormatBenchmark.cpp
        - decltype(auto) is used for format20Numbers()
    - folly/container/Foreach-inl.h
        - decltype(auto) and generic lambdas are used in a few of the
          for_each_range_impl() overloads
        - decltype(auto) is used in some of the impl() overloads
        - decltype(auto) is used as the return type for fetch()
    - folly/container/Iterator.h
        - decltype(auto) is used as the get_emplace_arg() and unwrap_emplace_arg() return type

A different way of solving the problem would be to add a special case
for Clang to the `integer_sequence` check, similar to what is done for
MSVC, but that seemed like partially papering over the problem. The best
solution would be to get those feature macros into libc++, but that
wouldn't help for already-released versions of LLVM.

    [0]: https://blogs.msdn.microsoft.com/vcblog/2014/06/06/c14-stl-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
    [1]: https://bugs.llvm.org/show_bug.cgi?id=18584
Closes https://github.com/facebook/folly/pull/810

Reviewed By: aary

Differential Revision: D7482813

Pulled By: yfeldblum

fbshipit-source-id: 36310acbc227bfe646d6fad877a406b8d57407e5
parent ec7690d7
...@@ -765,8 +765,8 @@ struct Synchronized : public SynchronizedBase< ...@@ -765,8 +765,8 @@ struct Synchronized : public SynchronizedBase<
std::piecewise_construct_t, std::piecewise_construct_t,
std::tuple<DatumArgs...> datumArgs, std::tuple<DatumArgs...> datumArgs,
std::tuple<MutexArgs...> mutexArgs, std::tuple<MutexArgs...> mutexArgs,
std::index_sequence<IndicesOne...>, index_sequence<IndicesOne...>,
std::index_sequence<IndicesTwo...>) index_sequence<IndicesTwo...>)
: datum_{std::get<IndicesOne>(std::move(datumArgs))...}, : datum_{std::get<IndicesOne>(std::move(datumArgs))...},
mutex_{std::get<IndicesTwo>(std::move(mutexArgs))...} {} mutex_{std::get<IndicesTwo>(std::move(mutexArgs))...} {}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment