lmi
[Top][All Lists]
Advanced

[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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]