Discussion :

[Arzar]

Bonjour à tous !

Je viens de tomber sur ce bout de code dans une librairie tierce que j'utilise :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
 
template<class T>
char const *
ConvertToString( T const & t )
{
  std::stringstream str;
  static std::string strOut;
  str << t;
  strOut = str.str();
  return strOut.c_str();
}

A votre avis, est-ce que ce genre de code (renvoi d'un pointeur sur un objet statique) est correct, robuste et sans vice caché ?

[3DArchi]

Y a pas mal de bibliothèques qui font ça. La seule chose c'est que chaque nouvel appel invalide l'adresse précédemment renvoyée. Donc, tu ne peux pas la garder pour un usage ultérieur.
Donc :
-> correct : il me semble que oui
-> robuste : non
-> vices cachés : la personne qui arrive après toi et qui n'a pas la curiosité de regarder l'implémentation et pense pouvoir garder le pointeur d'une fois sur l'autre

[Goten]

Quel est l'intérêt au final? Là j'avoue que je vois mal comment quelqu'un a pu se dire :
"tiens et si je renvoyais un pointeur sur une donnée statique interne à une fonction?"

[Arzar]

Merci !

-> correct : il me semble que oui
-> robuste : non
-> vices cachés : la personne qui arrive après toi et qui n'a pas la curiosité de regarder l'implémentation et pense pouvoir garder le pointeur d'une fois sur l'autre

Ok, j'en étais arrivé à peu près aux mêmes conclusions, merci pour la confirmation.

Quel est l'intérêt au final? Là j'avoue que je vois mal comment quelqu'un a pu se dire :
"tiens et si je renvoyais un pointeur sur une donnée statique interne à une fonction?"

Alors, j'ai inspecté un peu plus le code de la bibliothèque. En fait, je suspecte que son auteur pensait avoir trouvé une superbe feinte pour économiser des copies, mais qu'il a oublié dans son analyse la RVO/NRVO.

Il n'utilise la fonction ConvertToString que de deux manières :

Code :

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

1
2
3
4
5
6
7
8
9
 
// 1ere manière
std::string s = ConvertToString(data);
 
// 2eme manière
Truc truc( ConvertToString( data ) );
 
// avec
Truc::Truc(const std::string& value):value_(value){}

Donc il n'a pas de problème de durée de vie du pointeur car il copie systématiquement le retour de ConvertToString dans une std::string. Mais au final, il fait une copie de plus que le code de la faq :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
 
template<typename T>
std::string to_string( const T & Value )
{
    // utiliser un flux de sortie pour créer la chaîne
    std::ostringstream oss;
    // écrire la valeur dans le flux
    oss << Value;
    // renvoyer une string
    return oss.str(); // RVO possible ici.
}

qui exploite la RVO.

(Enfin, si je ne me suis pas planté en testant. Pour référence, je laisse ci-dessous mon code de test)

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
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
 
#include <iostream>
#include <string>
 
struct A
{
    A(){std::cout << "default ctor" << std::endl;}
    A(A* a){std::cout << "ctor A*" << std::endl;}
    A(const A& a){std::cout << "copy ctor" << std::endl;}
    A& operator=(const A& x){std::cout << "copy op=" << std::endl; return *this;}
    ~A(){std::cout << "dtor" << std::endl;}
};
 
struct B
{
    B(const A& a):a_(a){}
    B(A* a):a_(a){}
 
    A a_;
};
 
// fake stringstream
struct stringstream
{
    A str()
    {
        A a;
        // do something...
        return a;
    }
};
 
template<class T>
A* ConvertToAPtr( T const & t )
{
    static A strOut;
    stringstream str;
    // str << t;
    strOut = str.str();
    return &strOut;
}
 
template<class T>
A ConvertToA( T const & t )
{
    stringstream str;
    //str << t
    return str.str();
}
 
 
int main()
{
    std::cout << "ConvertToAPtr(T); (Warm up static data)\n";
    ConvertToAPtr(16);
 
    std::cout << "\nA a(ConvertToAPtr(T)\n";
    A a1(ConvertToAPtr(16));
 
    printf("\nB b(ConvertToAPtr(T)\n"); 
    B b1(ConvertToAPtr(15));
 
    printf("\nA a(ConvertToA(T)); (Note : NRVO here)\n");
    A a2(ConvertToA(16));
 
    std::cout << "\nB b(ConvertToA(T));\n";
    B b2(ConvertToA(15));
 
    printf("\nEND\n");
 
    return 0;
}

[3DArchi]

Alors, j'ai inspecté un peu plus le code de la bibliothèque. En fait, je suspecte que son auteur pensait avoir trouvé une superbe feinte pour économiser des copies, mais qu'il a oublié dans son analyse la RVO/NRVO.

C'est une technique que j'ai vu beaucoup employée dans du code C embarqué. Quelque chose du genre :

Code :

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

1
2
3
4
5
6
7
 
char const *do_something(/*...*/)
{
  static char result[TAILLE];
   /*...*/
  return result;
}

La zone mémoire est de taille fixe, il n'y a pas d'échec d'allocation possible et on connait l'occupation mémoire dès la compilation (et si c'est une variable globale, je crois qu'on peut même choisir son emplacement).
Ca peut donc être aussi quelqu'un qui a repris ses habitudes C en les appliquant à un code C++ où cela devient caduc voire, comme tu dis, pénalisant (car dans ton exemple il y aura allocation et recopie et on perd le bénéfice de la (N)RVO) .

[Médinoc]

En plus, ce n'est évidemment pas réentrant, et plus grave, avec une implémentation aussi "naïve", ce n'est pas thread-safe (il faudrait utiliser explicitement ou implicitement du Thread-Local Storage).

[ac_wingless]

Le renvoi d'un pointeur sur un objet statique renvoie bien la même chose d'une fois sur l'autre, mais dans le code présenté, ce qui est renvoyé n'est pas un "pointeur sur un objet statique interne à une fonction", mais le résultat d'une requête sur un objet (qui en l'occurrence est statique à une fonction, mais le problème ne vient pas de là).

Le résultat de cette requête peut être invalidé, comme pour bien des objets; c'est un peu comme si on renvoyait un vector<T>::begin(): cela n'est ni bon ni mauvais en soi, il faut simplement tenir compte de la durée de vie du pointeur en question. En l'occurrence, la valeur de retour deviendra fausse dès l'appel suivant, non pas parce que l'objet en question était local à une fonction, non pas parce que la procédure n'est pas thread-safe, mais tout simplement parce que le contenu de l'objet change, et donc que la résultat de la requête précédente devient invalide. En gros, ce code est exactement aussi faux que:

Code :

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

1
2
3
4
5
vector<int>  a;
    a.push_back(5);
vector<int>::const_iterator pa = a.begin();
    a.clear();
    cout << *pa;

Le code présenté est mauvais sans aucun doute, mais d'abord pour des raisons de parallélisme, et non pas pour des raisons de durée de vie du résultat: il est difficile de croire qu'un utilisateur déduirait de la signature de la fonction ("char const *ConvertToString( T const & t )") que le pointeur retourné a une durée de vie dépassant la séquence d'appel.

Il me semble donc qu'il y a dans les commentaires quelques confusions quant aux causes du problème: la non-réentrance est effectivement due à l'utilisation d'une variable statique interne à la fonction, ce qui correspond au titre de la discussion. Par contre, la non-conservation de la valeur n'est pas supposable d'après la signature, n'est pas due au fait que l'objet est statique et interne à la fonction, et est donc sans rapport avec le titre de la discussion.