New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added an assignment operator to SoundSource #864

Merged
merged 1 commit into from Sep 27, 2015

Conversation

Projects
None yet
5 participants
@mantognini
Member

mantognini commented Apr 13, 2015

Added an assignment operator to SoundSource.

See @minirop's PVS-Studio report

Regarding the implementation, I didn't write the "SFML standard" code, that is:

SoundSource& SoundSource::operator =(const SoundSource& right)
{
    SoundSource temp(right);

    std::swap(m_source, temp.m_source);

    return *this;
}

Because this would have implied editing the destructor too so that temp would be properly deallocated. I wanted to keep this PR as minimal as possible but let me know if you prefer something different.

@mantognini mantognini self-assigned this Apr 13, 2015

@mantognini mantognini added this to the 2.3 milestone Apr 13, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 13, 2015

Member

Because this would have implied editing the destructor too so that temp would be properly deallocated

What do you mean? If the destructor doesn't properly deallocates the class' resources, then there's a problem ;)

And something else: I don't like where you've put this operator =. I tend to put members in the following order:

  1. ctors
  2. dtor
  3. functions
  4. operators
Member

LaurentGomila commented Apr 13, 2015

Because this would have implied editing the destructor too so that temp would be properly deallocated

What do you mean? If the destructor doesn't properly deallocates the class' resources, then there's a problem ;)

And something else: I don't like where you've put this operator =. I tend to put members in the following order:

  1. ctors
  2. dtor
  3. functions
  4. operators
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 13, 2015

Member

What do you mean? If the destructor doesn't properly deallocates the class' resources, then there's a problem ;)

Actually, my reasoning was incorrect. Everything should fine.

(The invariant 'm_source is always valid during the lifetime of the object' is fine but I somehow thought it would not be the case with the implementation of = with swap...)

I don't like where you've put this operator =.

Fixed. ;-)

Member

mantognini commented Apr 13, 2015

What do you mean? If the destructor doesn't properly deallocates the class' resources, then there's a problem ;)

Actually, my reasoning was incorrect. Everything should fine.

(The invariant 'm_source is always valid during the lifetime of the object' is fine but I somehow thought it would not be the case with the implementation of = with swap...)

I don't like where you've put this operator =.

Fixed. ;-)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 13, 2015

Member

This is the copy-and-swap idiom (just with inline implementation of swap), it must work 😉

Something else: the derived class sf::Sound does not invoke the base class operator= in its own operator=. I assume the m_source handle is just recycled... is the implementation correct?

The fact that derived classes don't use sf::SoundSource::operator= is very likely the reason why the latter has not been implemented in the first place (I already wondered why it was not there, Laurent doesn't forget such stuff 😀). Should we rethink the semantics?

Member

Bromeon commented Apr 13, 2015

This is the copy-and-swap idiom (just with inline implementation of swap), it must work 😉

Something else: the derived class sf::Sound does not invoke the base class operator= in its own operator=. I assume the m_source handle is just recycled... is the implementation correct?

The fact that derived classes don't use sf::SoundSource::operator= is very likely the reason why the latter has not been implemented in the first place (I already wondered why it was not there, Laurent doesn't forget such stuff 😀). Should we rethink the semantics?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 14, 2015

Member

Should we rethink the semantics?

Good question but I've no idea.. @LaurentGomila?

Member

mantognini commented Apr 14, 2015

Should we rethink the semantics?

Good question but I've no idea.. @LaurentGomila?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 14, 2015

Member

Well... I don't remember 😐

Member

LaurentGomila commented Apr 14, 2015

Well... I don't remember 😐

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 14, 2015

Member

Here is the implementation of sf::Sound::operator=. Note the comment in the beginning:

Sound& Sound::operator =(const Sound& right)
{
    // Here we don't use the copy-and-swap idiom, because it would mess up
    // the list of sound instances contained in the buffers

    // Detach the sound instance from the previous buffer (if any)
    if (m_buffer)
    {
        stop();
        m_buffer->detachSound(this);
        m_buffer = NULL;
    }

    // Copy the sound attributes
    if (right.m_buffer)
        setBuffer(*right.m_buffer);
    setLoop(right.getLoop());
    setPitch(right.getPitch());
    setVolume(right.getVolume());
    setPosition(right.getPosition());
    setRelativeToListener(right.isRelativeToListener());
    setMinDistance(right.getMinDistance());
    setAttenuation(right.getAttenuation());

    return *this;
}

All the assigned attributes except loop and sound buffer are inherited from sf::SoundSource. I think we should remove the corresponding code and invoke sf::SoundSource::operator= in the derived class. The buffer assignment can be left as-is.

Member

Bromeon commented Apr 14, 2015

Here is the implementation of sf::Sound::operator=. Note the comment in the beginning:

Sound& Sound::operator =(const Sound& right)
{
    // Here we don't use the copy-and-swap idiom, because it would mess up
    // the list of sound instances contained in the buffers

    // Detach the sound instance from the previous buffer (if any)
    if (m_buffer)
    {
        stop();
        m_buffer->detachSound(this);
        m_buffer = NULL;
    }

    // Copy the sound attributes
    if (right.m_buffer)
        setBuffer(*right.m_buffer);
    setLoop(right.getLoop());
    setPitch(right.getPitch());
    setVolume(right.getVolume());
    setPosition(right.getPosition());
    setRelativeToListener(right.isRelativeToListener());
    setMinDistance(right.getMinDistance());
    setAttenuation(right.getAttenuation());

    return *this;
}

All the assigned attributes except loop and sound buffer are inherited from sf::SoundSource. I think we should remove the corresponding code and invoke sf::SoundSource::operator= in the derived class. The buffer assignment can be left as-is.

@mantognini mantognini modified the milestones: 2.4, 2.3 Apr 15, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 15, 2015

Member

You're probably right but I won't have time during the next week or so to do it so I changed the milestone to 2.4 in order not to block us further with this minor issue.

Member

mantognini commented Apr 15, 2015

You're probably right but I won't have time during the next week or so to do it so I changed the milestone to 2.4 in order not to block us further with this minor issue.

@mantognini mantognini removed their assignment Apr 30, 2015

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 8, 2015

Member

Do you guys agree on what I wrote on Apr 14, 2015? If so, I could adapt this commit correspondingly (assuming @mantognini won't mind, as he unassigned himself).

Member

Bromeon commented Jul 8, 2015

Do you guys agree on what I wrote on Apr 14, 2015? If so, I could adapt this commit correspondingly (assuming @mantognini won't mind, as he unassigned himself).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 8, 2015

Member

I don't mind. I wanted to review this PR and its decision soon but hadn't had time (and won't probably have time this week) so you're more than welcome to work on it. ;)

(Quickly glancing at your last comment, I still agree with you.)

Member

mantognini commented Jul 8, 2015

I don't mind. I wanted to review this PR and its decision soon but hadn't had time (and won't probably have time this week) so you're more than welcome to work on it. ;)

(Quickly glancing at your last comment, I still agree with you.)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 12, 2015

Member

I got rid of copy-and-swap in the base class, and used attribute-wise get/set combinations instead. The derived class' assignment operator calls the base class' one. For such changes, it would be really nice to verify with a test case that nothing breaks 😀

The amended commit still carries author information for mantognini, signed off by me. The original commit was cc2c5c5.

Member

Bromeon commented Jul 12, 2015

I got rid of copy-and-swap in the base class, and used attribute-wise get/set combinations instead. The derived class' assignment operator calls the base class' one. For such changes, it would be really nice to verify with a test case that nothing breaks 😀

The amended commit still carries author information for mantognini, signed off by me. The original commit was cc2c5c5.

@Bromeon Bromeon self-assigned this Jul 12, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 14, 2015

Member

At first sight it looks good. :-)

Member

mantognini commented Jul 14, 2015

At first sight it looks good. :-)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 16, 2015

Member

Could someone review this?

Member

eXpl0it3r commented Sep 16, 2015

Could someone review this?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 17, 2015

Member

Looks and works good for me. 👍

Member

binary1248 commented Sep 17, 2015

Looks and works good for me. 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 20, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Sep 20, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Sep 20, 2015

Added SoundSource::operator= and called it from Sound::operator=
Signed-off-by: Jan Haller <bromeon@gmail.com>

@eXpl0it3r eXpl0it3r removed this from the 2.4 milestone Sep 27, 2015

@eXpl0it3r eXpl0it3r merged commit 2d1fab3 into master Sep 27, 2015

16 checks passed

sfml-debian-64-gcc Build #327 succeeded in 1 min 27 sec
Details
sfml-osx Build #194 succeeded in 2 min 55 sec
Details
sfml-ubuntu-64-gcc Build #181 succeeded in 1 min 50 sec
Details
sfml-windows7-32-mingw492 Build #266 succeeded in 1 min 26 sec
Details
sfml-windows7-32-msvc10 Build #246 succeeded in 1 min 42 sec
Details
sfml-windows7-32-msvc11 Build #243 succeeded in 1 min 59 sec
Details
sfml-windows7-32-msvc12 Build #238 succeeded in 2 min 57 sec
Details
sfml-windows7-32-tdm471 Build #242 succeeded in 4 min 34 sec
Details
sfml-windows7-32-tdm481 Build #240 succeeded in 1 min 11 sec
Details
sfml-windows7-64-mingw492 Build #251 succeeded in 1 min 40 sec
Details
sfml-windows7-64-msvc10 Build #243 succeeded in 1 min 49 sec
Details
sfml-windows7-64-msvc11 Build #238 succeeded in 4 min 48 sec
Details
sfml-windows7-64-msvc12 Build #255 succeeded in 4 min 5 sec
Details
sfml-windows7-64-tdm471 Build #237 succeeded in 6 min 59 sec
Details
sfml-windows7-64-tdm481 Build #256 succeeded in 1 min 31 sec
Details
test Build #160 succeeded in 1 min 45 sec
Details

@eXpl0it3r eXpl0it3r added this to the 2.4 milestone Sep 27, 2015

@eXpl0it3r eXpl0it3r deleted the bugfix/pvs-report branch Sep 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment