Discussion :

[germinolegrand]

Salut à tou·te·s !

Je suis tombé aujourd'hui sur un warning, m'indiquant (à juste titre du point de vue du compilateur) que ma fonction pourrait ne pas faire de return.
Or il se trouve qu'arriver à la fin n'est algorithmiquement pas possible (à moins que mon objet ne soit dans un état corrompu).
J'ai donc voulu indiquer explicitement au compilateur qu'il était dans l'erreur (par ignorance).

Mais après plusieurs idées à base de [[noreturn]], et moult essais d'implémentation d'un éventuel assert_unreachable(), je suis dans l'impasse. Je me fends donc d'un joker, en retournant une valeur au hasard (bon, pas tant au hasard que cela, mais elle n'a aucun sens).

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
auto Parser::get_previous_node() -> decltype(m_results)::iterator
{
    if(m_results.empty())
        return end(m_results);
 
    int back_level = 0;
 
    for(auto it = end(m_results) - 1; it != begin(m_results); --it)
    {
        back_level += it->first;
 
        if(back_level == 0)
            return it;
        if(back_level > 0)
            return end(m_results);
    }
 
    assert("unreachable" == false);
    return end(m_results);
}

Ma tentative d'implémentation de assert_unreachable :

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
#pragma once
 
#include <cassert>
#include <cstdlib>
 
namespace fys
{
 
namespace priv
{
 
[[noreturn]] inline void assert_unreachable_do_nothing()
{}
 
}
 
#define assert_unreachable() \
    (assert(("unreachable", true == false)), \
    fys::priv::assert_unreachable_do_nothing())
 
}

Qui m'indique évidemment par un warning que ma fonction assert_unreachable_do_nothing() retourne quand même. Ce qui personnellement ne me dérangerait pas, mais on en revient au problème initial, comment faire comprendre au compilo que l'undefined behaviour est explicitement le but recherché, on tourne donc en rond.

Ici il s'agit d'un itérateur, il est donc possible de "tricher", mais en posant la problématique plus largement, je me demande comment gérer ce cas...

Avez-vous déjà rencontré de tels cas ?

Avez-vous des pistes pour implémenter un assert_unreachable ?

Merci !

[Iradrille]

Hello,

Tu peux sortir le return de la boucle

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
auto Parser::get_previous_node() -> decltype(m_results)::iterator {
	if(m_results.empty()) {
		return end(m_results);
	}
 
	int back_level = 0;
	auto it = end(m_results) - 1;
 
	for(; it != begin(m_results); --it) {
		back_level += it->first;
 
		if(back_level == 0) {
			break
		}
		else if(back_level > 0) {
			it = end(m_results);
			break
		}
	}
 
	return it;
}

Avec une meilleure condition de boucle on peut éviter les break.

Faut faire attention au cas m_results.size() == 1 qui renvoi end(m_results) dans ton code, et begin(m_results) dans le mien.

[Bousk]

Salut,

pourquoi ne pas faire un throw ?

[Luc Hermitte]

La solution du break est simple et efficace, malgré son arrière gout de SESE.

Il y a aussi moyen de ne pas mettre la condition de fin dans la boucle. Après tout cela semble cohérent avec le supposé invariant de la fonction : qu'on peut trouver une valeur avant la fin de la boucle.

Pour le throw, c'est faire de la programmation défensive d'une certaine façon. L'assert est plus dans la philo : "je sais que je ne suis pas sensé arriver là, si j'y arrive, j'ai une erreur de programmation et je préfère autant me payer un joli coredump".

Après, là, comme ça, je n'ai pas de solution au problème de faire savoir au compilo que l'on n'est pas sensé passer par là.

[white_tentacle]

Salut !

Pour le cas général, je n’ai pas de solution. J’ai le problème régulièrement avec des méthodes prenant en paramètre des enum et gérant toutes les valeurs « possibles » (disons, définies) dans un switch, sans clause default. Je suis obligé de définir un return X qui n’a pas de sens, je n’ai pas d’autre solution.

Pour ton cas particulier, en revanche, quelques remarques :
- le code tel que je lis me fais fortement soupçonner un bug sur le « it != begin(m_results) », je pencherai plutôt pour un >=, si on souhaite vraiment ignorer le premier élément de la liste, de mon point de vue ça mérite un commentaire .
- l’invariant qui garantit la terminaison précoce de la boucle me semble complexe à garantir. Es-tu sûr qu’il est nécessairement respecté ? (ie il ne dépend pas d’un jeu de données supposé correct).
- je rejoins Luc sur le fait que l’exception se rapproche du défensif. Néanmoins, ça reste une forme acceptable d’UB au même titre qu’un assert. Vu qu’on parle de code qui ne sera jamais exécuté, je privilégierai plutôt l’assert (l’exception pourrait entraîner un overhead dans le code généré…).

[koala01]

Salut,

A titre personnel, quand je suis dans cette situation, je me contente d'un assert (du type de assert(!"You should never come here") et je renvoie un itérateur connu pour être invalide (collection.end() ) pour "faire taire" le compilo

Mais bon, s'il y a moyen de modifier l'algorithme pour avoir le retour effectif en fin de fonction (et "tant pis pour le cote SESE "), je le fais aussi régulièrement

[skeud]

Salut, dans le code de base, il y a un truc que je ne comprends pas, et c'est peut-etre pour ça que le compilo te crache un warning.

Ton it->first, semble être un int, donc il peut avoir une valeur négative, donc back_level peut être négatif donc, ne jamais passer par un return.

Ensuite si ton algo te définis que it->first est toujours positif, alors pourquoi faire une boucle? Tu t’arrêtera toujours à la première itération car le seul moyen d'en faire une deuxième est que back_level soit < 0.

Je me trompe?

[n0-sheep]

Salut, pourquoi ne pas utiliser abort() ?

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
 
auto Parser::get_previous_node() -> decltype(m_results)::iterator
{
    if(m_results.empty())
        return end(m_results);
 
    int back_level = 0;
 
    for(auto it = end(m_results) - 1; it != begin(m_results); --it)
    {
        back_level += it->first;
 
        if(back_level == 0)
            return it;
        if(back_level > 0)
            return end(m_results);
    }
    abort(); //unreachable
}