Discussion :

[Pyramidev]

Bonjour,

Dans le cas où le compilateur ne peut pas générer automatiquement le code de l'affectation de mouvement, peut-on considérer comme une bonne pratique de développement le fait de factoriser le code de l'affectation de mouvement en appelant explicitement le destructeur puis le constructeur de mouvement ?

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
Foo& operator=(Foo&& other) noexcept
{
    if(this != &other) {   // allows std::swap<Foo>(x,x)
        this->Foo::~Foo(); // "Foo::" prevents virtuality if the destructor is virtual.
        new(this) Foo(std::move(other));
    }
    return *this;
}

Voici un exemple très concret où je trouve que cette pratique est pertinente.
Admettons qu'une personne code un modèle de classe MovableRaiiFileStream<FileStreamType> avec un membre de type FileStreamType* et qui gère l'ouverture et la fermeture :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
template<class FileStreamType>
class MovableRaiiFileStream
{
private:
    FileStreamType* m_file;
 
    bool invariant()
    {
        return m_file != nullptr;
    }
public:
    MovableRaiiFileStream(FileStreamType& file, const std::string& path) :
        m_file(&file)
    {
        file.open(path);
        if(!file.is_open())
            throw std::runtime_error("Unable to open file \"" + path + "\".");
    }
    MovableRaiiFileStream(const MovableRaiiFileStream& other)            = delete;
    MovableRaiiFileStream& operator=(const MovableRaiiFileStream& other) = delete;
    MovableRaiiFileStream(MovableRaiiFileStream&& other) noexcept :
        m_file(other.m_file)
    {
        other.m_file = nullptr;
    }
    ~MovableRaiiFileStream() noexcept
    {
        try {
            if(m_file)
                m_file->close();
        } catch(...) {
            try {
                std::cerr << "Unable to close file.";
            } catch(...) {
            }
        }
    }
    MovableRaiiFileStream& operator=(MovableRaiiFileStream&& other) noexcept
    {
        if(this != &other) {
            this->MovableRaiiFileStream::~MovableRaiiFileStream();
            new(this) MovableRaiiFileStream(std::move(other));
        }
        return *this;
    }
};

Ensuite, admettons que cette personne se dise que ce serait mieux que MovableRaiiFileStream<FileStreamType> soit responsable de gérer la mémoire du FileStreamType et que ce serait bien aussi de garder en mémoire le chemin du fichier. D'ailleurs, ce chemin pourra être affiché en cas d'échec de fermeture du fichier. Le code devient alors :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
template<class FileStreamType>
class MovableRaiiFileStream
{
private:
    std::unique_ptr<FileStreamType> m_file;
    std::string                     m_path;
 
    bool invariant()
    {
        return bool(m_file);
    }
public:
    explicit MovableRaiiFileStream(const std::string& path) :
        m_file(new FileStreamType(path)), m_path(path)
    {
        if(!m_file->is_open())
            throw std::runtime_error("Unable to open file \"" + path + "\".");
    }
    MovableRaiiFileStream(MovableRaiiFileStream&& other) noexcept = default;
    ~MovableRaiiFileStream() noexcept
    {
        try {
            if(m_file)
                m_file->close();
        } catch(...) {
            try {
                std::cerr << "Unable to close file \"" << m_path << "\".";
            } catch(...) {
            }
        }
    }
    MovableRaiiFileStream& operator=(MovableRaiiFileStream&& other) noexcept
    {
        if(this != &other) {
            this->MovableRaiiFileStream::~MovableRaiiFileStream();
            new(this) MovableRaiiFileStream(std::move(other));
        }
        return *this;
    }
};

De la première à la deuxième version du code, le code du constructeur de mouvement a été supprimé (car le compilateur pouvait le générer) et celui du destructeur a changé. Mais celui de l'affectation de mouvement est inchangé, grâce à la factorisation.

Pour l'instant, le principal inconvénient qui me vient à l'esprit à propos de cette pratique, c'est que le code ne sera compréhensible que par ceux qui connaissent le placement new.

Qu'en pensez-vous ?

[Astraya]

Ou tout simplement:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
 
MovableRaiiFileStream& operator=(MovableRaiiFileStream&& other) noexcept
{
    m_file = other.m_file;
    m_path = std::move(other.m_path);
    return *this;
}

[Pyramidev]

Ou tout simplement:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
 
MovableRaiiFileStream& operator=(MovableRaiiFileStream&& other) noexcept
{
    m_file = other.m_file;
    m_path = std::move(other.m_path);
    return *this;
}

Il y a trois problèmes avec ton code :

1. m_file = other.m_file ne compile pas, car std::unique_ptr n'a pas d'affectation de copie. Tu voulais probablement écrire m_file = std::move(other.m_file).
2. Tu ne fais que réimplémenter l'affectation de mouvement par défaut. Donc autant écrire directement :
   

   Code :

   Sélectionner tout - Visualiser dans une fenêtre à part

   MovableRaiiFileStream& operator=(MovableRaiiFileStream&& other) noexcept = default;

3. [EDIT 22h14]Enfin, le plus gros problème, c'est que ton affectation de mouvement (alias affectation de mouvement par défaut) n'essaie pas de fermer le fichier de this avant de prendre en charge celui de other. L'objectif de la classe n'est donc pas rempli. Par contre, mon affectation de mouvement appelle explicitement le destructeur qui essaie bien de fermer le fichier de this.
   Erreur de ma part. J'avais cru que les destructeur de std::basic_ifstream et de std::basic_ofstream ne fermaient pas le fichier.
   Cependant, implémenter soi-même un destructeur qui ferme le fichier permet de choisir quoi faire en cas d'échec de fermeture. J'ai écrit dans std::cerr, mais j'aurais pu faire quelque chose de plus intéressant, comme écrire dans un fichier de log en précisant le niveau de log.[/EDIT]

[Astraya]

Un coup c'est un pointer, un coup c'est un unique_ptr... Je vois pas quel debat tu veux avoir. Ta solution est... "Degeulasse" pardonne moi l'expression.
Pour le unique_ptr tu appelle reset(). Sinon tu delete le pointer avant de l'affecter. C'est tout. Le reste est sans intérêt.

[Pyramidev]

L'intérêt de factoriser le code de l'affectation de mouvement en appelant explicitement le destructeur puis le constructeur de mouvement, c'est de ne pas avoir besoin de changer le code de l'affectation de mouvement lorsque le code du destructeur et/ou celui du constructeur de mouvement change.

Je vois pas quel debat tu veux avoir.

J'avais fait une recherche sur internet à propos de ma solution.
Quelqu'un avait évoqué cette solution ici : http://stackoverflow.com/questions/2...ve-constructor

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
Class& operator=(Class&& rhs)
{
    this->~Class();                    // call destructor
    new (this) Class(std::move(rhs));  // call move constructor in-place
}

Les deux contre-arguments qu'il avait reçu étaient :

1. Le code n'était pas adapté dans le cas où le destructeur était virtuel (corrigeable en remplaçant this->~Class() par this->Class::~Class()).
2. Le code risquait d'être difficile à maintenir à cause de la syntaxe assez rare. Je l'ai évoqué dans mon premier message : « le principal inconvénient qui me vient à l'esprit à propos de cette pratique, c'est que le code ne sera compréhensible que par ceux qui connaissent le placement new ».

A part ça, j'avais lu un vieux Guru of the week du 11 octobre 1997 qui critiquait l'implémentation de l'affectation de copie en fonction en fonction du destructeur et du constructeur de copie :
http://www.gotw.ca/gotw/023.htm
Mais cela ne me semblait pas transposable à l'affectation de mouvement.

J'ai publié ce fil ici au cas où la technique évoquée dans mon premier message aurait des inconvénients qui m'auraient échappés.

[dragonjoker59]

Ben avec le unique_ptr, aucun intérêt à utiliser ta méthode, car si le FileStreamType est bien fait (RAII tout ça), il fermera le fichier à sa destruction et c'est réglé...

[Pyramidev]

Ben avec le unique_ptr, aucun intérêt à utiliser ta méthode, car si le FileStreamType est bien fait (RAII tout ça), il fermera le fichier à sa destruction et c'est réglé...

T'as publié quelques minutes après mon édition de mon 2e message. On s'est croisé.

Erreur de ma part. J'avais cru que les destructeur de std::basic_ifstream et de std::basic_ofstream ne fermaient pas le fichier.
Cependant, implémenter soi-même un destructeur qui ferme le fichier permet de choisir quoi faire en cas d'échec de fermeture. J'ai écrit dans std::cerr, mais j'aurais pu faire quelque chose de plus intéressant, comme écrire dans un fichier de log en précisant le niveau de log.