[libcxx-commits] [PATCH] D114761: [libc++][ranges] Implement [special.mem.concepts].

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 12 inline comments as done.
var-const added inline comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
ldionne wrote:
> cjdb wrote:
> > Would you mind renaming this file to `uninitialized_algo_concepts.h` or similar please?
> Let's try to avoid forking the header naming discussion happening below! Given your comment, I guess it makes sense to take the suggestion below and rename it to `__memory/concepts.h` -- that should satisfy everyone.
Renamed to `concepts.h`.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:33
+    input_iterator<_Ip> &&
+    // Not a proxy iterator.
+    is_lvalue_reference_v<iter_reference_t<_Ip>> &&
----------------
Quuxplusone wrote:
> ldionne wrote:
> > tcanens wrote:
> > > ldionne wrote:
> > > > Quuxplusone wrote:
> > > > > ldionne wrote:
> > > > > > var-const wrote:
> > > > > > > Quuxplusone wrote:
> > > > > > > > I recommend deleting this comment line.
> > > > > > > What's the rationale for deleting this comment? If you feel it is incorrect or incomplete, I'd rather improve wording than delete it altogether. It took me some time to figure out the meaning of the additional constraints which I hoped this comment could save for somebody else.
> > > > > > So this comment actually comes from a discussion Costa and I had where we were wondering what this `is_lvalue_reference_v<iter_reference_t<_Ip>> && same_as<remove_cvref_t<iter_reference_t<_Ip>>, iter_value_t<_Ip>>` business was. We came to the common understanding that they were trying to prevent proxy iterators.
> > > > > > 
> > > > > > Does that make sense to you? If so, I think the comment is useful cause it was not entirely obvious to us at first.
> > > > > Mainly the comment line is just confusing because it's so terse: we could rewrite it to `__nothrow_input_iterator accepts input iterators that aren't proxy iterators`, but I'm not sure that's literally true.
> > > > > 
> > > > > I can certainly design something that I'd call a "proxy iterator" that happens to return `T&` from its `operator*` — like, maybe it's like `istream_iterator` and stashes the referred-to thingie inside itself.
> > > > > In fact, `__nothrow_input_iterator<std::istream_iterator<int>> == true` today, right?
> > > > > 
> > > > > I would say that this restriction comes from the fact that we're going to be calling `std::addressof(*it)`, so `*it` needs to be an lvalue: prvalues and xvalues can't be passed to `addressof`.
> > > > > But the most immediate rationale is simply "because we copy-pasted this line directly from the Standard." I don't think we //need// to insert our own speculations. Maybe turn it into a commit-message comment? :)
> > > > FWIW my impression is that those requirements were there so that we know there aren't other potentially throwing operations when one does `iter_reference_t<Iterator> ref = *it`, since that would kind of circumvent the whole point of the concept describing an iterator that doesn't throw. If for example initializing `iter_reference_t<Iterator>` did throw, then you'd end up with a potentially throwing statement in a place where the user probably never expected one, since the concept is `__nothrow_input_iterator`. So my understanding was that when they spec'd it out, they said "let's prevent any subtlety from happening by requiring that these `__nothrow_iput_iterator`s return lvalue references, nothing fancier.
> > > > 
> > > > If the rationale is really just the syntactical requirement of doing `std::addressof(*it)`, then honestly this concept is incredibly mis-named.
> > > > 
> > > > @tcanens Do you know why the following requirements are added to the concept:
> > > > 
> > > > ```
> > > > is_lvalue_reference_v<iter_reference_t<_Ip>> &&
> > > > same_as<remove_cvref_t<iter_reference_t<_Ip>>, iter_value_t<_Ip>>
> > > > ```
> > > > 
> > > > Whatever the answer is, I think it would be useful to record that in a comment, given the discussion we're having right now.
> > > > 
> > > This is for constructing into where `it` points to (or for the input version in particular, used only for `destroy(_n)`, destroying what `it` points to). I don't see how we can implement it if it's not a real reference.
> > Ok, that makes sense, and admittedly I had lost the original purpose of these concepts when I formulated my hypothesis about non-throwing oprations. But then the concept is incredibly badly named. This has way more to it than not throwing exceptions. I suggest we change the comment to:
> > 
> > ```
> > // This is used to ensure that uninitialized algorithms can construct an object
> > // at the address pointed-to by the iterator, which requires an lvalue.
> > is_lvalue_reference_v<iter_reference_t<_Ip>> &&
> > same_as<remove_cvref_t<iter_reference_t<_Ip>>, iter_value_t<_Ip>>;
> > ```
> > 
> > At least that makes it clear this is unrelated to throwing exceptions.
> I like that wording but let's keep it out of the middle of the expression, please.
> ```
> // This concept ensures that uninitialized algorithms can construct an object
> // at the address pointed-to by the iterator, which requires an lvalue.
> template <class _Ip>
> concept __nothrow_input_iterator =
>     input_iterator<_Ip> &&
>     is_lvalue_reference_v<iter_reference_t<_Ip>> &&
>     same_as<remove_cvref_t<iter_reference_t<_Ip>>, iter_value_t<_Ip>>;
> ```
> [or simply `// at addressof(*it)`, but if that reopens a whole discussion, nvm :)]
Done. @tcanens Thanks for the explanation!


================
Comment at: libcxx/include/module.modulemap:633
     module __memory {
-      module addressof                { private header "__memory/addressof.h"                }
-      module allocation_guard         { private header "__memory/allocation_guard.h"         }
-      module allocator                { private header "__memory/allocator.h"                }
-      module allocator_arg_t          { private header "__memory/allocator_arg_t.h"          }
-      module allocator_traits         { private header "__memory/allocator_traits.h"         }
-      module auto_ptr                 { private header "__memory/auto_ptr.h"                 }
-      module compressed_pair          { private header "__memory/compressed_pair.h"          }
-      module construct_at             { private header "__memory/construct_at.h"             }
-      module pointer_traits           { private header "__memory/pointer_traits.h"           }
-      module raw_storage_iterator     { private header "__memory/raw_storage_iterator.h"     }
-      module shared_ptr               { private header "__memory/shared_ptr.h"               }
-      module temporary_buffer         { private header "__memory/temporary_buffer.h"         }
-      module uninitialized_algorithms { private header "__memory/uninitialized_algorithms.h" }
-      module unique_ptr               { private header "__memory/unique_ptr.h"               }
-      module uses_allocator           { private header "__memory/uses_allocator.h"           }
+      module addressof                         { private header "__memory/addressof.h"                       }
+      module allocation_guard                  { private header "__memory/allocation_guard.h"                }
----------------
cjdb wrote:
> As a general thing, I didn't bother to align the closing braces when making the initial header splits, and don't care for this change. It's very brittle on the LHS, and I don't want to duplicate said brittleness.
Done. I'm also not a fan of these kinds of formatting -- I originally did it just for consistency with the existing code.


================
Comment at: libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_iterator.compile.pass.cpp:25-27
+  bool operator==(const ForwardIterator_Proxy&) const;
+
+  int operator*() const;
----------------
Quuxplusone wrote:
> Another trivial nit here: I'd delete blank line 26 and swap the order of lines 25 and 27.
In this case, I'd prefer to keep it as-is. I deliberately separated `operator*` because it's the only function of interest in this class.


================
Comment at: libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_sentinel_for.compile.pass.cpp:21
+#include "test_range.h"
+#include "MoveOnly.h"
+
----------------
jloser wrote:
> Is this include needed? Same with `"test_range.h"`, etc.
Thanks for spotting. I cleaned up other test files but missed this one.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.input/input_iterator.compile.pass.cpp:26
 
-  no_explicit_iter_concept() = default;
-
----------------
cjdb wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > Why are we removing those here?
> > > This file had much more refactoring in the first revision, and then I said "let's split out all the controversial refactoring into a new PR." This is just what was left over. IMO this file should be landed in its own separate commit, and that could be done right now today AFAIC.
> > I think it would make sense to split this in a separate diff and explain why we're removing those. It's probably a one liner to explain it, but I'd like it on record.
> Agreed. Please keep commits small and relevant.
Reverted


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114761/new/

https://reviews.llvm.org/D114761