[libcxx-commits] [PATCH] D105794: [libcxx][algorithms] adds ranges::is_partitioned and ranges::partition_point

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 2 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/include/__algorithm/partition_point.h:71
+    _LIBCPP_NODISCARD_EXT constexpr
+    _Ip operator()(_Ip __first, const _Sp __last, _Pred __pred, _Proj __proj = {}) const {
+      // This assertion breaks the logarithmic complexity requirement, but since it is only evaluated
----------------
Quuxplusone wrote:
> Mordante wrote:
> > cjdb wrote:
> > > Mordante wrote:
> > > > Why the `const` in `const _Sp __last`. (I personally like it, but it deviates from our coding style.)
> > > I like the compiler catching accidental writes. Also, this is a new section of code, so I think it's worth piloting this style here. (@ldionne seems to be ambivalent towards it based on prior commits as evidence.)
> > I like the `const` too for the the same reason. So then let's keep it. I probably will start to use it in new code too.
> @cjdb @Mordante @ldionne: Please, please, don't misuse `const` to "pass by const value" or "return by const value." These are common typos in C++ code. @zoecarver will (I hope) recall the couple of times I've caught bugs in his code by grepping for pass-by-const-value typos.
> 
> I have a whole blog post on "const" here:
> https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/
> 
> and it ends with [a set of `git grep` regexes](https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/#grep-your-codebase-today) that enable anyone — not just me, but //any of you!// — to find your missing-ampersand typos by grepping for parameters of the form `const _Sp __last`. If you start using that typo-style in code you //don't// want to change, it makes the job of the typo-finder vastly harder.
> 
> As of this writing, we have only five instances of pass-by-const-value in the codebase. Three of them are in `requires`-clauses.
> ```
> $ git grep '[(,] *const [A-Za-z0-9_:]* [A-Za-z0-9_:]* *[,)=]'
> __iterator/advance.h:      if (const auto __M = __bound - __i; __abs(__n) >= __abs(__M)) {
> __iterator/concepts.h:  requires(const _In __i) {
> __iterator/concepts.h:  requires(_Ip __i, const _Ip __j, const iter_difference_t<_Ip> __n) {
> __iterator/iter_swap.h:  requires(const _I1 __i1, const _I2 __i2) {
> random:binomial_distribution<_IntType>::param_type::param_type(const result_type __t, const double __p)
> ```
I've read through this a handful of times now. I don't find it particularly convincing. You claim that they're "common typos", but I can only find one concrete example that you've come across in that blog, which doesn't generalise. I can't speak to Zoe's experience here.

Yes, `const`-qualifying value parameters //can// be a problem. It's not absolute truth that they //all// are. You wrote that "`const` is a contract". I agree with this: hell, I even teach it, but it seems we have a fundamental point of contention on this topic. Your blog implies that it's only a contract between a programmer and the interface they're programming against. From this perspective, yes, a `const` value parameter is useless, but this is not the only way in which `const` is a contract. `const` is also a contract between the programmer and the compiler. Under this lens, a `const` value parameter has strong meaning, because it gives both the reader and writer strong confidence that the object isn't going to be modified.

> and it ends with [a set of git grep regexes](https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/#grep-your-codebase-today) that enable anyone — not just me, but //any of you//! — to find your missing-ampersand typos by grepping for parameters of the form `const _Sp __last`. If you start using that typo-style in code you don't want to change, it makes the job of the typo-finder vastly harder.

This isn't as strong an argument against `const` value parameters as you think it is. It effectively reads as "let's weaken our code guarantees so that the person auditing our codebase has an easy time doing their audit". If you want an effective and painless auditing experience, write a tool that automates the process and has a configurable set of exemptions or set of rules.


================
Comment at: libcxx/include/__algorithm/partition_point.h:79
+
+      auto __distance = 0;
+      for (auto __i = __first; __i != __last; ++__i) ++__distance; // FIXME: replace with ranges::distance
----------------
Needs to be `std::iter_difference_t<_Ip>{0}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105794