Problème avec vector::push_back - Page 1

J'ai une classe token définie comme ça:

class token
{
private:
	TOKENS _type;
	void *_data;
public:
	token(TOKENS type, std::string *text);
	token(TOKENS type, int number);
	token(TOKENS type, double number);
	token(TOKENS type, void *data);
	~token();
	TOKENS getType();
	void *getData();
};

avec ceci comme destructeur:

token::~token()
{
	if (_data != NULL)
		free(_data);
}

Je stocke une collection d'objets token dans une classe vector<token> (rien d'anormal jusque là)
type stringsi je fais vector::push_back(*new token(TOK_FILE, filename)); // filename étant de , le destructeur de mon objet token passé en argument est appelé
et au bout de qq appels similaires, le prog plante
j'aimerais pas rester bloqué là dessus
EDIT: en +, avec un destructeur vide, ya pas de plantage Mais pourquoi est-ce que ce satané destructeur est appelé ?

Ton objet est passé par valeur, donc une copie est créée, et la copie temporaire retournée par new et passée hors de l'endroit de validité ("scope") est détruite. Ta classe a besoin d'un constructeur de copies ("copy constructor"). Presque toutes les classes qui ont besoin d'un destructeur ont aussi besoin d'un constructeur de copies et d'un opérateur d'assignement (=) surchargé.

En effet, ça fonctionne maintenant
merci bcp

Moué :
- *new Foo ne détruira pas la mémoire allouée pour l'objet Foo et créera un memory leak; il faut faire Foo() à la place... (à moins que ça ne soit juste pour le testcase ?)
- que contient _data ? pkoi est-ce que tu utilises malloc() et pas new() pour le créer ? et est-ce que tu ne t'en tirerais pas plus facilement/proprement avec un union ou bien un template plutôt que de faire des casts à partir de "void *" ?
- et tu utilises trop de pointeurs... Pourquoi "std::string *" et pas "const std::string &" ?

Kevin> operator= n'a pas besoin d'être surchargé, en général...

De plus :
- NULL n'existe pas en C++, on utilise 0
- on préfère mettre la section "public" en premier dans la classe, pour une lecture immédiate de l'interface
- si TOKENS n'est pas une macro, éviter les majuscules
- éviter les '_' au début des identifiants, on peut toujours s'en sortir sans :

token::token(TOKENS type, void *data)
{
  this->data = data;
}

ou encore mieux :

token::token(TOKENS type, void *data): data(data)
{
}

Pollux
: - que contient _data ? pkoi est-ce que tu utilises malloc() et pas new() pour le créer ?

Pourquoi pas? Si malloc lui convient, il peut l'utiliser.

- et tu utilises trop de pointeurs... Pourquoi "std::string *" et pas "const std::string &" ?

Peut-être parce qu'un pointeur est plus facile à comprendre qu'une référence constante?

Kevin> operator= n'a pas besoin d'être surchargé, en général...

Si! Réfléchis un peu! Tu en as besoin pour exactement la même raison que le constructeur de copie: si tu as des pointeurs allouées à l'extérieur, tu ne peux pas seulement copier les pointeurs, tu dois aussi copier les contenus, et le langage C++ ne fait pas ça pour toi. Et tu dois aussi désallouer les anciens pointeurs avant d'écrire les nouveaux par dessus.

nitro :
De plus : - NULL n'existe pas en C++, on utilise 0

Faux. NULL existe en C++ est est défini dans cstddef/stddef.h.

Kevin Kofler
:

nitro
: - NULL n'existe pas en C++, on utilise 0

Faux. NULL existe en C++ est est défini dans cstddef/stddef.h.

s/n'existe pas/est rarement utilisé/ car il a posé problème avec un bon nombre de compilateurs (conflit avec la définition du C, qui n'est pas la même). En C++ NULL est strictement équivalent à 0.
Avec g++ ce problème ne se pose pas.

Kevin Kofler
:

- et tu utilises trop de pointeurs... Pourquoi "std::string *" et pas "const std::string &" ?

Peut-être parce qu'un pointeur est plus facile à comprendre qu'une référence constante?

Non. "const Type &" est la méthode classique pour passer un paramètre constant (on peut aussi se contenter de passer "Type", mais c moins efficace). Et c'est bien plus naturel sémantiquement de passer une valeur qu'une adresse, à moins de vraiment vouloir garder une référence par la suite... (mais c'est déjà bcp plus spécifique, comme utilisation)

Kevin> operator= n'a pas besoin d'être surchargé, en général...

Si! Réfléchis un peu! Tu en as besoin pour exactement la même raison que le constructeur de copie: si tu as des pointeurs allouées à l'extérieur, tu ne peux pas seulement copier les pointeurs, tu dois aussi copier les contenus, [...]. Et tu dois aussi désallouer les anciens pointeurs avant d'écrire les nouveaux par dessus.

Je me doute bien

et le langage C++ ne fait pas ça pour toi

Argl, tu as raison, qd on ne définit pas de operator=(), celui créé par défaut se contente de tout recopier et n'utilise pas le copy constructor s'il y en a un (comme je le pensais) Mais je n'ai pas rencontré ce pb puisque 95% des classes que j'ai faites ne gèrent pas la mémoire elle-même (donc le comportement par défaut est identique à ce que j'ai fait), et les 5% restants sont les smart pointers & co, et il se trouve que j'avais écrit un operator=() aussi

token::~token()
{
	if (_data != NULL)
		free(_data);
}

free(NULL); est parfaitement legal (Ne fait rien).

nitro :
s/n'existe pas/est rarement utilisé/ car il a posé problème avec un bon nombre de compilateurs (conflit avec la définition du C, qui n'est pas la même). En C++ NULL est strictement équivalent à 0. Avec g++ ce problème ne se pose pas.

Mais cela reste indispensable parfois (void f(int i, ...); par exemple.).
Et le probleme est qu'il n'y a plus de cast automatique de void * en type *...

PpHd :

token::~token()
{
	if (_data != NULL)
		free(_data);
}

free(NULL); est parfaitement legal (Ne fait rien).

Tiens, c'est vrai Alors TIGCC se comporte de manière illégale de ce point de vue-là...

nitro :
s/n'existe pas/est rarement utilisé/ car il a posé problème avec un bon nombre de compilateurs (conflit avec la définition du C, qui n'est pas la même). En C++ NULL est strictement équivalent à 0. Avec g++ ce problème ne se pose pas.

Mais cela reste indispensable parfois (void f(int i, ...); par exemple.).

Les fonctions à nombre variable d'arguments, c'est MAL.

Et le probleme est qu'il n'y a plus de cast automatique de void * en type *...

En C++, les void *, c'est MAL.

En C++, les void *, c'est MAL.

Il parait. Mais je me demande toujours pourquoi.

Parce que template, parce que classe héritée, parce que union.

Ben on peut voir void * comme la classe de base de l'ensemble des pointeurs, non ?

"union"
Pourquoi union ?

PpHd
: Ben on peut voir void * comme la classe de base de l'ensemble des pointeurs, non ?

Le but est aussi de ne pas rajouter d'information de typage dynamique, hein (contrairement à ce qui se passe qd on a une vraie classe de base)

"union" Pourquoi union ?

Parce que :
class {
int type;
void *foo;
};
->
class {
int type;
union {
Expr *expr;
Type *type;
} foo;
};

(et je soupçonne que dans le cas de GoldenCrystal ça soit suffisant)

C'est MAL ce que tu proposes comme solution. L'heritage a ete fait pour ne pas ecrire ce genre de choses.

Oui, mais ça alourdit considérablement les choses...

Et puis la solution propre serait que ses différentes classes pour les différents types de tokens soient toutes dérivées d'une même classe de base, et si possible en n'ayant même plus de pointeurs (sauf exception)

Pollux
: - que contient _data ? pkoi est-ce que tu utilises malloc() et pas new() pour le créer ? et est-ce que tu ne t'en tirerais pas plus facilement/proprement avec un union ou bien un template plutôt que de faire des casts à partir de "void *" ?

Ben _data contient soit NULL, soit un pointeur vers une donnée de type quelquonque qui ne regarde pas l'objet. J'utilise malloc parce que ce n'est pas un pointeur vers un objet, mais vers un bloc de mémoire associé. Une union ? je pense que je vois ce que tu veux dire, en effet, c'est possible. Une template? je ne vois pas l'intérêt dans ce cas.

- et tu utilises trop de pointeurs... Pourquoi "std::string *" et pas "const std::string &" ?

Heu... à vrai dire, je n'en ai aucune idée J'avais tout d'abord écrit le code avec une référence, et pour une raison x dont je ne me souviens plus, je l'ai remplacé par un pointeur. Mais ça fait revient quasiment au même de toute façon

nitro
: - NULL n'existe pas en C++, on utilise 0

Hmm, c possible, mais je préfère mettre NULL quand même. Et puis dans tous les cas, si jamais ça posait des problèmes, ce n'est pas le + difficile à corriger (soit par une #define NULL 0, soit par un remplacement de texte )

- on préfère mettre la section "public" en premier dans la classe, pour une lecture immédiate de l'interface

J'y penserait Mais ça me paraissait plus naturel de mettre la section private en premier

- si TOKENS n'est pas une macro, éviter les majuscules

TOKENS est un énum

- éviter les '_' au début des identifiants, on peut toujours s'en sortir sans :

Hmm, ba j'utilise généralement '_' pour les déclarations privées dans tous les languages

PpHd
: free(NULL); est parfaitement legal (Ne fait rien).

Ah, ok, je vais virer le test alors

...

En ce qui concerne l'héritage, je ne sais pas si c'est vraiment utile (ça serait évidemment plus dans l'esprit C++ ), car ça n'améliorerait pas le reste du programme. (Il y aurait toujours besoin de token::getType() pour connaître le type des données, peu importe comment je m'y prends )

GoldenCrystal
:

Pollux
: - que contient _data ? pkoi est-ce que tu utilises malloc() et pas new() pour le créer ? et est-ce que tu ne t'en tirerais pas plus facilement/proprement avec un union ou bien un template plutôt que de faire des casts à partir de "void *" ?

Ben _data contient soit NULL, soit un pointeur vers une donnée de type quelquonque qui ne regarde pas l'objet. J'utilise malloc parce que ce n'est pas un pointeur vers un objet, mais vers un bloc de mémoire associé. Une union ? je pense que je vois ce que tu veux dire, en effet, c'est possible. Une template? je ne vois pas l'intérêt dans ce cas.

Si ça ne regarde pas du tout ton objet (i.e. l'application pourrait y mettre un map<string,pair<float,double> > *), alors ça serait plutôt un template qu'il faudrait utiliser, non? Et sinon, ce serait plutôt une union...

Et puis j'imagine que tu as bien du code qui caste à partir du void* pour en faire un type utilisable...

Et puis j'imagine que tu as bien du code qui caste à partir du void* pour en faire un type utilisable...

ba oui, mais ce ne sont pas des classes, juste des types de base
Extrait:

	tokenizer &t = *new tokenizer(*new string(argv[1]));
	tokenList &l = t.tokenize(f);
	for (i = 0; i < l.size(); i++) {
		if ((l[i].getType() == TOK_IDENTIFIER) ||
			(l[i].getType() == TOK_LABEL) ||
			(l[i].getType() == TOK_FILE))
			cout << tokens[l[i].getType()] << " (\"" << (char *)l[i].getData() << "\")" << endl;
		else if ((l[i].getType() == TOK_INTNUMBER) || (l[i].getType() == TOK_LINE))
			cout << tokens[l[i].getType()] << " (" << *(int *)l[i].getData() << ")" << endl;
		else if (l[i].getType() == TOK_FPNUMBER)
			cout << tokens[l[i].getType()] << " (" << *(double *)l[i].getData() << ")" << endl;
		else if ((l[i].getType() == TOK_STR) || (l[i].getType() == TOK_FILE))
			cout << tokens[l[i].getType()] << " (\"" << (char *)l[i].getData() << "\")" << endl;
		else
			cout << tokens[l[i].getType()] << endl;
	}

Euh, et tokens est un tableau de char*

Oui, c'est bien ce que je dis : ton "void *" est inefficace...

Ce serait tellement plus simple de faire un token<T> qui dérive de tokenBase, et qui contiendrait des données auxiliaires de type T (dans ton cas, T sera soit "int", soit "double", soit "std::string"). C'est même plus efficace au niveau vitesse et mémoire, parce que tu n'as pas à faire des malloc(sizeof(int)) pour stocker un nombre...

PpHd
: free(NULL); est parfaitement legal (Ne fait rien).

Sur une plateforme conforme aux standards, oui. Sous un vieux Unix, ou sous AMS, non.

Pollux
:

PpHd
: free(NULL); est parfaitement legal (Ne fait rien).

Tiens, c'est vrai Alors TIGCC se comporte de manière illégale de ce point de vue-là...

Oui, mais c'est la faute de AMS.

GoldenCrystal
:

- éviter les '_' au début des identifiants, on peut toujours s'en sortir sans :

Hmm, ba j'utilise généralement '_' pour les déclarations privées dans tous les languages

Mauvaise idée. Il y a des restrictions sur les identifiants commençant par '_' en C/C++, parce qu'ils sont fait pour les déclarations privées de l'implémentation (compilateur, linker, librairie système etc.). (Les restrictions sont moins strictes pour les variables en '_'+minuscule, ils ne sont réservés que dans certains contextes. En revanche, les variables en '_'+majuscule ou '__' sont totalement tabous. D'ailleurs, en C++, la séquence '__' est taboue même au milieu, parce qu'elle est réservée pour le mangling de noms.)

Kevin> bah non, le std ne pose pas de pb pour "_data"... (mais c vrai qu'il faut aussi faire attention à ne pas prendre __data ou _Data)

Kevin> Ca prend 2s a corriger dans les headers de tigcc...

La dernière fois que j'ai proposé de faire ça, il y avait la moitié de la communauté pour me gueuler dessus que ça rend les programmes inefficaces, alors il faut savoir... Sebastian aussi était plutôt contre à l'époque. Nous ne sommes pas la seule plateforme non-conforme sur ce point. Le manuel de portabilité de GCC (pour la programmation de GCC lui-même) avertit spécialement:

free and realloc
----------------

Some implementations crash upon attempts to free or realloc the null
pointer.  Thus if mem might be null, you need to write

  if (mem)
    free (mem);

95% de la communaute est inculte et n'a aucun sens des realites."programmes inefficaces" a cause de cela

G un autre pb maintenant (en fait, je l'avais déjà avant, mais maintenant qu'il devient gênant... )
Dans une fonction, j'ai ce code:

	tokenList &l = *new tokenList();
	string line;
	// autres déclarations

	l.push_back(*new token(TOK_FILE, _filename));
	while (!f.eof()) {
		getline(f, line);
		// suite de la boucle
	}

f est de type istream (il évidemment ouvert)
Le problème là dedans, c'est que quand je fais getline, line reste vide et f.eof() passe à true Pourtant le fichier n'est pas vide
Je c pas si j'ai fait une fausse manip, mais en tout cas, je n'ai pas trouvé la cause de ce problème.

vtp

cf ./4, push_back(*new Type(args)) --> push_back(Type(args))


Et pour ton pb, il faut tester "f.eof()" _après_ la lecture (comme en C).

Pollux :
vtp

J'ai pas de corde assez longue

cf ./4, push_back(*new Type(args)) --> push_back(Type(args))

Erf, mais comment est-ce que j'ai pu oublier ça Pourtant à une époque, je le savais #suicide#

Et pour ton pb, il faut tester "f.eof()" _après_ la lecture (comme en C).

Euh, ça ne résoud rien (logique d'ailleurs, vu qu'au début f.eof() est toujours egual à false).

GoldenCrystal
:

cf ./4, push_back(*new Type(args)) --> push_back(Type(args))

Erf, mais comment est-ce que j'ai pu oublier ça Pourtant à une époque, je le savais #suicide#

Et tu pourras alors en profiter pour supprimer le & de tokenList... (sinon, bobo à la mémoire)

Et pour ton pb, il faut tester "f.eof()" _après_ la lecture (comme en C).

Euh, ça ne résoud rien (logique d'ailleurs, vu qu'au début f.eof() est toujours egual à false).[/cite]
Argl, mal lu. Tu es sûr que f est bien ouvert?

^^ Erf, c bon, g touvé le problème. J'avais l'aissé l'ancien prototype de la fonction (qui prenait un paramètre f) dans la classe et du coup ça passait par dessus le membre f de ma classe (ça m'apprendra à pas mettre de _ sur un membre privé )