[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [lmi] [lmi-commits] master 8862ffc 7/9: Improve const correctness
From: |
Greg Chicares |
Subject: |
Re: [lmi] [lmi-commits] master 8862ffc 7/9: Improve const correctness |
Date: |
Thu, 13 Jun 2019 00:00:48 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
On 2019-06-12 17:26, Vadim Zeitlin wrote:
> On Wed, 12 Jun 2019 12:53:00 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> GC> commit 8862ffce33cd76fb09a5a5850d058a3807be9a17
[...]
> GC> Improve const correctness
> GC>
> GC> Replaced this idiom:
> GC> https://isocpp.org/wiki/faq/ctors#named-parameter-idiom
> GC> with a const variant that suffices for all real-world use cases.
[...]
> These const methods indeed seem to be sufficient, but for someone used to
> the named parameter idiom use (such as me), they look quite confusing. I
I hadn't remembered it as a standard idiom until I searched for it.
I've added some comments reflecting your suggestions.
> think it would be nice to add a comment to the database_index class
> explaining that we do _not_ use this idiom here. Alternatively, I'd
> consider naming these functions new_with_foo() instead of just foo(), to
> make it more clear that create a new object rather than modify the existing
> one. This would make the code using them much longer, of course...
[...]> GC> mcenum_state state () const {return
static_cast<mcenum_state >(idx_[5]);}
[...]
> GC> + database_index state (mcenum_state z) const {auto i = idx_;
> i[5] = z; return {i};}
>
> Another possible improvement could be to add symbolic names instead of
> using the hard-coded 0..5 constants here. This again would make the code
> slightly longer but I think it's worth it, "i[5]" really doesn't look very
> self-documenting.
The lines exceed eighty characters, so they're too long already.
(E.g., look at the way it's displayed
https://lists.nongnu.org/archive/html/lmi/2019-06/msg00019.html
in the html archives; or imagine embedding such a patch inline
in email.)
We could
- mcenum_gender gender () const {return static_cast<mcenum_gender
>(idx_[0]);}
+ auto gender () const {return static_cast<mcenum_gender >(idx_[0]);}
which would shorten the first block of long lines, but it's easier
to see a function's return type if it's explicitly stated.
We could reimplement members like state(mcenum_state), in the second
block of long lines, in terms of a member function template, e.g.:
- database_index state (mcenum_state z) const {auto i = idx_; i[5] = z;
return {i};}
+ database_index gender (mcenum_gender z) const {return
change<e_axis_gender>(z);}
but that would add complexity, without getting us to eighty columns.
The only way to get to eighty columns is to replace 'database_index'
with a drastically less clear name, like 'di'.
This thing is regrettably metastable, but I can't think of a good way
to change it again without making it worse.