search

dcleblanc / SafeInt

SafeInt is a class library for C++ that manages integer overflows.
MIT License
216 stars 37 forks source link

Fix clang diagnostic about deprecated generated copy constructor. #32

Closed jaykrell closed 3 years ago

jaykrell commented 3 years ago

Fix implicit copy constructor clang 10.0 diagnostic (-Weverything, that we use).

.../safeint.hpp.original:5741:35: error: definition of implicit copy constructor for 'SafeInt<long, SafeIntInternal::SafeIntExceptionHandler >' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy] _CONSTEXPR14 SafeInt< T, E >& operator =( const SafeInt< T, E >& rhs ) SAFEINT_NOTHROW ^ .../safeint.hpp.original:6536:12: note: in implicit copy constructor for 'SafeInt<long, SafeIntInternal::SafeIntExceptionHandler >' first required here return SafeInt<ptrdiff_t, SafeIntDefaultExceptionHandler>( p1 - p2 ); ^ There are many ways to fix this.

  1. Suppress it: 
#ifdef __clang__
#pragma clang diagnostic ignored "-Wdouble-promotion"
#pragma clang diagnostic ignored "-Wdeprecated-copy"
#endif

...

#ifdef __clang__
#pragma clang diagnostic pop
#endif

  1. We can remove the operator= and let the compiler supply the default for both. This seems like a good idea. That is done here. It is smallest source, and works and has the same meaning in C++98 and newer.

  2. We can provide copy constructor, that the compiler was providing anyway:

    _CONSTEXPR11 SafeInt(const SafeInt& 
other) SAFEINT_NOTHROW : 
m_int(other.m_int) { }

    Experimentation suggests the compiler-supplied copy constructor is not explicit. i.e. this program:

class A
{
public:

A(int) { }

explicit
A(const A&) {}

void operator=(const A&) { } // warning here, depending on if previous present

};

A f1();
A f1()
{
return A(1);
}

add/remove the copy constructor, and add/remove explicit. It does compile, with warning, w/o the copy constructor. It does not compile if the copy constructor is explicit.

This is ok, but then we have two manually provided functions, for which the compiler can produce the same thing. One small difference, is these can be more easily stepped though.

  1. We can declare both, but say they are "=default", a C++11 feature. This is really the same as option 2, let the compiler do it, albeit slightly explicit. This would require conditionality on C++11 vs. 98.
dcleblanc commented 3 years ago

If you’re talking about these:

template < typename U >
_CONSTEXPR14 SafeInt< T, E >& operator =( const SafeInt< U, E >& rhs ) SAFEINT_CPP_THROW
{
    SafeCastHelper< T, U, GetCastMethod< T, U >::method >::template CastThrow< E >( rhs.Ref(), m_int );
    return *this;
}

_CONSTEXPR14 SafeInt< T, E >& operator =( const SafeInt< T, E >& rhs ) SAFEINT_NOTHROW
{
    m_int = rhs.m_int;
    return *this;
}

The first is by design and needed. I’m not sure if the default implementation will take the place of the second – it may.

From: Jay Krell notifications@github.com Sent: Tuesday, December 8, 2020 10:08 PM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [dcleblanc/SafeInt] Fix clang warning about deprecated generated copy constructor. (#32)

Fix implicit copy constructor clang 10.0 diagnostic (-Weverything, that we use).

.../safeint.hpp.original:5741:35: error: definition of implicit copy constructor for 'SafeInt<long, SafeIntInternal::SafeIntExceptionHandler >' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy] _CONSTEXPR14 SafeInt< T, E >& operator =( const SafeInt< T, E >& rhs ) SAFEINT_NOTHROW ^ .../safeint.hpp.original:6536:12: note: in implicit copy constructor for 'SafeInt<long, SafeIntInternal::SafeIntExceptionHandler >' first required here return SafeInt<ptrdiff_t, SafeIntDefaultExceptionHandler>( p1 - p2 ); ^ There are many ways to fix this.

  1. Suppress it:

ifdef clang pragma clang diagnostic ignored "-Wdouble-promotion" pragma clang diagnostic ignored "-Wdeprecated-copy" endif

...

ifdef clang pragma clang diagnostic pop endif

  1. We can remove the operator= and let the compiler supply the default for both. This seems like a good idea. That is done here. It is smallest source, and works and has the same meaning in C++98 and newer.
  2. We can provide copy constructor, that the compiler was providing anyway: _CONSTEXPR11 SafeInt(const SafeInt& other) SAFEINT_NOTHROW : m_int(other.m_int) { }

Experimentation suggests the compiler-supplied copy constructor is not explicit. i.e. this program:

class A

{

public:

A(int) { }

explicit

A(const A&) {}

void operator=(const A&) { } // warning here, depending on if previous present

};

A f1();

A f1()

{

return A(1);

}

add/remove the copy constructor, and add/remove explicit. It does compile, with warning, w/o the copy constructor. It does not compile if the copy constructor is explicit.

This is ok, but then we have two manually provided functions, for which the compiler can produce the same thing. One small difference, is these can be more easily stepped though.

  1. We can declare both, but say they are "=default", a C++11 feature. This is really the same as option 2, let the compiler do it, albeit slightly explicit. This would require conditionality on C++11 vs. 98.

You can view, comment on, or merge this pull request online at:

https://github.com/dcleblanc/SafeInt/pull/32

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/pull/32, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YLZLWA2T776CLDOGLVTST4H35ANCNFSM4UTAAMDA.

jaykrell commented 3 years ago

It is the second. The assignment of same SafeInt type to same SafeInt type. I understand you intend them both to be there, and this PR does not really change that. What this does, is take away the manual implementation of the second, letting the compiler generate the same.

For example:

class A
{
public:
 void operator=(int); // assignment from some other type
};

void f()
{
A a;
A b;
a = b; // does not stop assignment from same type from working
}

and in doing so, taking away the manual implementation, gives the compiler free reign to also generate the copy constructor. Which it already was doing. But this way, without entering into deprecated territory that gets a warning.

There is no intended semantic change, nor allow any code to compile that did not, or not compile that did, except to fix the warning.

dcleblanc commented 3 years ago

I'll try seeing if = default fixes it. I don't agree that this is worth a warning, but since it should be low risk, we can see. This code is already requiring C++ 11, so I have no problem with using features that came up in the last 9 years.

jaykrell commented 3 years ago

Yes, =default should also work.

I suggest doing it for copy constructor and operator=.

But again, note that removing the operator= does not really remove it. The compiler will just generate the same thing for you, equivalent or perhaps better.

The committee actually deprecated the idea of user providing operator= or copy constructor, but not both. They want you to provide both or neither. The compiler is just doing the committee's bidding here. SafeInt is unusual in only providing one of them.

dcleblanc commented 3 years ago

Fixed in last commit to use_c_headers branch

dcleblanc commented 3 years ago

So, I’ve gone through the PRs and issues, and I don’t see anything else outstanding other than the multiplication work, which I will do next, in a different check-in. Please let me know if there’s any other items that need attention, otherwise I’ll just verify that its all good on both gcc and clang, and merge this branch into main.

From: Jay Krell notifications@github.com Sent: Wednesday, December 9, 2020 12:50 AM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: David LeBlanc dcl@dleblanc.net; Comment comment@noreply.github.com Subject: Re: [dcleblanc/SafeInt] Fix clang diagnostic about deprecated generated copy constructor. (#32)

Yes, =default should also work.

I suggest doing it for copy constructor and operator=.

But again, note that removing the operator= does not really remove it. The compiler will just generate the same thing for you, equivalent or perhaps better.

The committee actually deprecated the idea of user providing operator= or copy constructor, but not both. They want you to provide both or neither. The compiler is just doing the committee's bidding here. SafeInt is unusual in only providing one of them.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/pull/32#issuecomment-741628011, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YL2H3I4IT7NNMPH3ASDST424LANCNFSM4UTAAMDA.