Re: [lmi] const or reference members

[Greg Chicares]

On 7/12/22 23:17, Vadim Zeitlin wrote:
> On Tue, 12 Jul 2022 22:26:27 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 7/12/22 21:40, Vadim Zeitlin wrote:
> GC> > On Tue, 12 Jul 2022 16:18:34 +0000 Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
> GC> > 
> GC> > GC> On 7/11/22 22:12, Vadim Zeitlin wrote:
> GC> > GC> > On Mon, 11 Jul 2022 16:22:14 +0000 Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
> GC> > GC> > 
> GC> > GC> > GC> On 7/11/22 12:12, Vadim Zeitlin wrote:
> GC> [...]
> GC> > GC> > GC> Here, I want to consider a distinct question: if, for any 
> given class,
> GC> > GC> > GC> defaulted copy operations are correct, then can default 
> assignment
> GC> > GC> > GC> operations be incorrect?
> GC> > GC> > 
> GC> > GC> >  It's, of course, possible to maliciously design just such a 
> class, but I
> GC> > GC> > don't think it's something that can happen organically.
> GC> > GC> 
> GC> > GC> I stumbled on one case: const reference members. I suppose either
> GC> > GC> const-ness or reference-ness is enough to poison assignment.
> GC> > 
> GC> >  Either is enough for the assignment not to be possible, but for me this
> GC> > doesn't mean it's incorrect -- it's disallowed, which is something quite
> GC> > different (and not nearly as bad).
> GC> 
> GC> Let me paraphrase, to see whether I understand:
> GC> 
> GC> Given a class with const or reference members, in our editor we can add
> GC> defaulted assignment operators, but our compiler will reject them.
> 
>  I might be nitpicking,

Not at all. Words matter a great deal.

> but the compiler won't "reject" them in the sense
> that it will result in any kind of error. The compiler will handle the
> default assignment operator for a non-assignable class in the same way as
> the deleted assignable operator because, for a non-assignable class, the
> default behaviour is to disallow assignment.

The saddest words in the language standard are "no diagnostic required".

Let's see what our compilers do with this patch:

--8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/gpt_server.hpp b/gpt_server.hpp
index 524be42e2..431b75bd5 100644
--- a/gpt_server.hpp
+++ b/gpt_server.hpp
@@ -66,6 +66,8 @@ class LMI_SO gpt_server final
     double seconds_for_output      () const;
 
   private:
+    int i {0};
+    int& ri {i};
     mcenum_emission emission_        {mce_emit_nothing};
     gpt_state state_                 {};
     double seconds_for_input_        {0.0};
--8<----8<----8<----8<----8<----8<----8<----8<--

gcc-11: it compiles and links, with no diagnostics.
However, clang says:

/opt/lmi/src/lmi/gpt_server.hpp:53:17: error: explicitly defaulted copy 
assignment operator i
s implicitly deleted [-Werror,-Wdefaulted-function-deleted]
    gpt_server& operator=(gpt_server const&) = default;
                ^
/opt/lmi/src/lmi/gpt_server.hpp:70:10: note: copy assignment operator of 
'gpt_server' is impl
icitly deleted because field 'ri' is of reference type 'int &'
    int& ri {i};
         ^
/opt/lmi/src/lmi/gpt_server.hpp:54:17: error: explicitly defaulted move 
assignment operator i
s implicitly deleted [-Werror,-Wdefaulted-function-deleted]
    gpt_server& operator=(gpt_server&&) = default;
                ^
/opt/lmi/src/lmi/gpt_server.hpp:70:10: note: move assignment operator of 
'gpt_server' is impl
icitly deleted because field 'ri' is of reference type 'int &'
    int& ri {i};
         ^

> GC> It won't accept them and generate invalid code.
> 
>  It certainly won't generate invalid code because any attempt to use the
> assignment operator will result in a compilation error.

<false_alarm ignore=1>

That describes the behavior of a compliant compiler, which is the context
of these abstract conversations.

However, gcc-11 compiles, links, and runs with the poisonous patch above,
and even produces the same output as it does without that patch. I thought
that was surprising, so I rebuilt with "build_type=ubsan", and the same
outcome. I ran a '.gpt' file in both the GUI and the CLI, and both ways
gave me a valid output file. Can gcc-11 be that far wrong?

No. I picked the wrong thing to test. I poisoned the assignment operators
in class gpt_server, but earlier in this thread I showed that lmi works
even if we explicitly declare them as deleted. They aren't used.

</false_alarm>

> GC> I've written classes with const or reference members. I'm wondering
> GC> whether I should repent and change them all.
> 
>  I definitely don't think there is any need to do it. Sometimes it may be
> worth using a pointer rather than a reference just to allow the class to be
> assignable, but if you don't especially need it to be assignable, you
> certainly shouldn't hesitate to use const or reference members when they're
> appropriate.
> 
> GC> OTOH, I recently found a case where changing a non-const non-reference
> GC> member to "const&" would permit a forward declaration where an entire
> GC> header is now included:
> GC> 
> GC>   -#include "round_to.hpp"
> GC>   +template<typename> class round_to;
> GC> 
> GC>   ...and change 'round_to' to 'round_to const&' in these places:
> GC>   $ git grep 'round_to<\w*> *\w*;' *.hpp
> GC> 
> GC> I resisted the temptation to make that change for fear it would
> GC> interfere with special-member operations.
> 
>  This looks like a potentially useful change, but looking at e.g.
> outlay.hpp in more details I see that you need to pass round_to<>
> objects to its ctor, which probably means that any code using it would
> include round_to.hpp anyhow. Unless it already uses references itself, of
> course... I.e. more analysis is needed to confirm that this will actually
> reduce physical dependencies (and I still didn't finish my experiments with
> IWYU that would have allowed to do things like this semi-automatically...).

Thanks very much for this alternative perspective.