Jump to conversation
Unresolved conversations (26)
@cjdb cjdb Jul 29, 2024
Nit: pass by value (same elsewhere for types that you wouldn't ordinarily pass by ref-to-const).
libcxx/test/support/test_iterators.h
jamesETsmith
@cjdb cjdb Jul 29, 2024
```suggestion _LIBCPP_HIDE_FROM_ABI _LIBCPP_ALWAYS_INLINE static constexpr iota_result<ranges::borrowed_iterator_t<_Range>, _Tp> ```
libcxx/include/__numeric/ranges_iota.h
philnik777 jamesETsmith
strega-nil
@var-const var-const May 14, 2024
Nit: `s/danderous/dangerous/`.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
@var-const var-const May 14, 2024
+1 to @Zingam's suggestion to mark this as `|Partial|`.
Outdated
libcxx/docs/Status/Cxx23Papers.csv
@var-const var-const Mar 31, 2024
Question: why do we need a base class to achieve this?
libcxx/test/support/test_iterators.h
cjdb jamesETsmith
@var-const var-const Mar 31, 2024
Nit: `s/inthe/The/`? Also, `s/outputrange/output range/` or `s/outputrange/output_range/`, etc.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@var-const var-const Mar 31, 2024
Nit: consider something like ``` This class has a "mischievous" non-const overload of copy-assignment operator that modifies the object being assigned from. `ranges::iota` should not be invoking this overload thanks to the `std::as_const` in its implementation. ```
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@var-const var-const Mar 31, 2024
Nit: can the name be either `all_snake_case` or `AllCamelCase`?
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@var-const var-const Mar 31, 2024
Perhaps I'm missing something, but it seems like it shouldn't return anything (the return type is `void`)?
Outdated
libcxx/test/support/test_iterators.h
jamesETsmith
@var-const var-const Mar 31, 2024
Question: why is it necessary to have a derived class? Could we just use `NonStandardLayoutTypeBase`?
...orithms.results/out_value_result.pass.cpp
@var-const var-const Mar 31, 2024
Nit: `s/initiazliation/initialization/`.
...orithms.results/out_value_result.pass.cpp
strega-nil
@var-const var-const Mar 31, 2024
Nit: consider passing the value explicitly (honestly, I think `MoveOnly` having a default non-zero value is a bit confusing).
...orithms.results/out_value_result.pass.cpp
@var-const var-const Mar 31, 2024
Hmm, rather than make this conditional, perhaps we can create a new object for this case? ``` // `iota` takes the argument by value, so make sure move-only types work (without moving from `x`) test(std::ranges::iota, in, T{}); ``` I think the reason these tests use `x` is just to make them nicer to read.
Outdated
...s_robust_against_proxy_iterators.pass.cpp
strega-nil jamesETsmith
@var-const var-const Mar 31, 2024
Nit: seems to be an extra space after the opening `<` bracket (unless it's added by clang-format).
Outdated
...s/ranges_robust_against_dangling.pass.cpp
jamesETsmith
@var-const var-const Mar 31, 2024
`s/In Progress/Complete` (which it will be once this patch is merged, right?).
Outdated
libcxx/docs/Status/RangesMajorFeatures.csv
strega-nil jamesETsmith
Zingam
@var-const var-const Mar 31, 2024
`s/P2400/P2440/`.
Outdated
libcxx/docs/Status/Cxx23Papers.csv
jamesETsmith
@var-const var-const Mar 31, 2024
`s/P2400/P2440/`.
Outdated
libcxx/docs/Status/Cxx23.rst
jamesETsmith
@cjdb cjdb Jan 5, 2024
What is the intention of this change?
Outdated
libcxx/test/support/test_iterators.h
@cjdb cjdb Jan 5, 2024
What is the intention of this change?
Outdated
libcxx/test/support/test_iterators.h
jamesETsmith cjdb
@cjdb cjdb Jan 5, 2024
These may be better off in a local header, since they're very `ranges::iota`-specific.
Outdated
libcxx/test/support/test_iterators.h
jamesETsmith
@cjdb cjdb Jan 5, 2024
Please move this into the iterator-based `operator()` and have the range-based one call that. As it's currently implemented, additional and unnecessary moves need to happen, and we also incur some debug overhead that I'd rather avoid.
Outdated
libcxx/include/__numeric/ranges_iota.h
jamesETsmith
@philnik777 philnik777 Jan 5, 2024
```suggestion types::for_each(types::cpp20_input_iterator_list{}, []<class Iter> { test_results<Iter>(); }); test_results<cpp17_output_iterator<int*>>(); test_results<cpp20_output_iterator<int*>>(); ``` You should also test with different sentinel types.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@philnik777 philnik777 Jan 5, 2024
I don't think this is required. They should both qualify as output iterators.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@philnik777 philnik777 Oct 7, 2023
You'll probably run into problems with this when updating the `robust_against_*` tests. You should move the algorithm itself into a helper.
Outdated
libcxx/include/__numeric/ranges_iota.h
jamesETsmith var-const
cjdb
@philnik777 philnik777 Oct 7, 2023
```suggestion static constexpr iota_result<_Out, _Tp> operator()(_Out __first, _Sent __last, _Tp __value) { ``` same below
Outdated
libcxx/include/__numeric/ranges_iota.h
jamesETsmith philnik777
@philnik777 philnik777 Oct 7, 2023
I've started working on this in https://reviews.llvm.org/D121436 (and didn't really work on it because nobody knew what it was for). You should probably make sure that any comments there are addressed here and add any additional tests from there.
...orithms.results/out_value_result.pass.cpp
Resolved conversations (30)
@cjdb cjdb Jul 29, 2024
Rather than having concepts, we should move these tests into a `.verify.cpp` file. requires-expressions fail silently, so we don't get signal here on why something like `HasIotaIter<InputIteratorNotInputOrOutputIterator>` is false: only that it is. In a verification test file, we can use Clang to inspect the diagnostic and confirm that `std::ranges::iota(std::forward<Iter>(iter), std::forward<Sent>(sent), std::forward<Value>(val))` fails for the intended reason reason.
...ric.ops/numeric.iota/ranges.iota.pass.cpp
cjdb jamesETsmith
philnik777
@strega-nil strega-nil Jul 25, 2024
Matter of style, but I'd personally prefer this to look like: ```suggestion return operator()(ranges::begin(__r), ranges::end(__r), std::move(__value)); ``` up to you.
Outdated
libcxx/include/__numeric/ranges_iota.h
@strega-nil strega-nil Jul 25, 2024
Same question as above, reason for the names in the parameters?
Outdated
libcxx/test/support/test_iterators.h
@strega-nil strega-nil Jul 25, 2024
Is there a reason you added names to these parameters?
Outdated
libcxx/test/support/test_iterators.h
jamesETsmith
@strega-nil strega-nil Jul 25, 2024
I think this might be a merge thing, this should probably go after `fold_left_with_iter_result` on new line 68.
Outdated
...esult_alias_declarations.compile.pass.cpp
jamesETsmith
@strega-nil strega-nil Jul 25, 2024
These tests seem good, but in a future PR it might be nice to centralize testing for all `_result` types (see also previous thought about centralizing the `_result` types' definitions). No change requested.
...orithms.results/out_value_result.pass.cpp
@strega-nil strega-nil Jul 25, 2024
It is not standard practice here to re-export the `*_result` types in the modules that use them; see, for example, `std_private_algorithm_ranges_for_each` not re-exporting `in_fun_result`.
libcxx/include/module.modulemap
strega-nil jamesETsmith
@strega-nil strega-nil Jul 25, 2024
nit: this should go below `in_value_result`, as it is later in both the standard and is alphabetically later.
Outdated
libcxx/include/algorithm
strega-nil
@strega-nil strega-nil Jul 25, 2024
I would personally prefer if these template type parameters were named `template <class _Out, class _Value>` or something along those lines (with the parameters below named `<class _Out2, class _Value2>`); no change requested, feel free to ignore.
...xx/include/__algorithm/out_value_result.h
strega-nil philnik777
@strega-nil strega-nil Jul 25, 2024
This accidentally doubles `contains` from a merge conflict; also, instead of adding a new line, you should edit line 10 instead.
Outdated
libcxx/docs/Status/RangesAlgorithms.csv
@var-const var-const May 14, 2024
We need to test both in `constexpr` mode and at runtime, so this should be: ``` test_results(); static_assert(test_results()); ``` It's a pattern we follow in general -- it's always possible for code to have different behavior between the two modes, whether due to `if consteval`, compiler bugs, etc.
...ric.ops/numeric.iota/ranges.iota.pass.cpp
var-const
@var-const var-const Mar 31, 2024
I'm missing something. I thought the non-const assignment should _not_ be selected, but `expected` values being different from the original implies that it is?
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith var-const
@var-const var-const Mar 31, 2024
Can we also test in constexpr mode (`static_assert(test_results());`) -- `test_results` would have to be `constexpr` and `return true`?
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@var-const var-const Mar 31, 2024
Nit: please include `<utility>`.
...ric.ops/numeric.iota/ranges.iota.pass.cpp
var-const jamesETsmith
@var-const var-const Mar 31, 2024
Nit: please sort the includes alphabetically.
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@cjdb cjdb Jan 5, 2024
Please use `std::weakly_incrementable` and `std::incrementable`. You'll also need to support `void operator++(int)` for when a type is weakly incremenetable, but not incrementable.
Outdated
libcxx/test/support/test_iterators.h
jamesETsmith
@cjdb cjdb Jan 5, 2024
Please move this conditional logic into a separate test case.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@cjdb cjdb Jan 5, 2024
Please avoid using logic in test cases.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
@cjdb cjdb Jan 5, 2024
Similar to the `__iota` namespace, this is likely going to be removed. Please track the discussion in #76542.
Outdated
libcxx/include/__numeric/ranges_iota.h
jamesETsmith
@cjdb cjdb Jan 5, 2024
This namespace isn't necessary, and I'm working on a patch to remove the ones that are already checked in. Since this patch is happening concurrently, would you mind removing this namespace here? Alternatively, you can track #76543 and hold off removing this until that lands in main.
Outdated
libcxx/include/__numeric/ranges_iota.h
jamesETsmith
@philnik777 philnik777 Jan 5, 2024
I'd also like to see a test with a non-fundamental type, and especially one that behaves differently when it's `const` to test the `std::as_const` in the loop.
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith var-const
@philnik777 philnik777 Jan 5, 2024
```suggestion assert(result.value == starting_value + N); ``` I think this should be equivalent.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@philnik777 philnik777 Jan 5, 2024
These test don't need to be inside a function. You can just put them at global scope. Otherwise they look really nice.
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
jamesETsmith
@philnik777 philnik777 Jan 5, 2024
The formatting changes in here are nice, but would be better handled in a separate PR, just to make it clear what actually changed.
Outdated
...s/ranges_robust_against_dangling.pass.cpp
jamesETsmith cjdb
@philnik777 philnik777 Dec 19, 2023
Please also add a note in `libcxx/docs/Status/Cxx23.rst` which states what parts are currently implemented.
Outdated
libcxx/docs/Status/Cxx23Papers.csv
jamesETsmith
@philnik777 philnik777 Oct 7, 2023
Please also update `libcxx/docs/RangesAlgorithms.csv`.
Outdated
libcxx/docs/Status/Cxx23Papers.csv
@philnik777 philnik777 Oct 7, 2023
Newline!
Outdated
...orithms.results/out_value_result.pass.cpp
@philnik777 philnik777 Oct 7, 2023
Newline!
Outdated
...ric.ops/numeric.iota/ranges.iota.pass.cpp
@philnik777 philnik777 Oct 7, 2023
Unrelated: Please update the `robust_against_*` tests.
...orithms.results/out_value_result.pass.cpp
@philnik777 philnik777 Oct 7, 2023
```suggestion #include <__utility/move.h> ```
libcxx/include/__numeric/ranges_iota.h