Discussion :

[Bktero]

Bonjour,

SonarCube (installé récemment sur un de nos serveurs) râle sur du code écrit il y a longtemps, et je ne suis plus sûr de pourquoi le code avait été écrit ainsi. Je vais faire appel à vos lumières sur ce coup-là

J'ai une classe template :

Code :

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

1
2
template<typename T, std::uint8_t integer, std::uint8_t fractional>
class FixedPoint { ....}

Il y une seule donnée membre dans cette classe : T raw_data;.

Parmi toutes les fonctions membres, il y a :

Code :

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

1
2
3
void swap(FixedPoint& other) {
	std::swap(raw_data, other.raw_data);
}

Enfin, dans le même fichier hpp, il y a la spécialisation de std::swap() dans le namespace std :

Code :

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

1
2
3
4
5
6
7
namespace std {
 
template<typename T, std::uint8_t integer, std::uint8_t fractional>
void swap(data::FixedPoint<T, integer, fractional>& lhs, data::FixedPoint<T, integer, fractional>& rhs) {
	lhs.swap(rhs);
}
}

SonarCube se plaint parce que j'utilise le namespace std justement. Pourtant, dans les explications du warning, il me dit :

A program may add a template specialization for any standard library template to namespace std only if the declaration depends on a user-defined type and the specialization meets the standard library requirements for the original template and is not explicitly prohibited.

J'ai l'impression de respecter cette règle. La seule hypothèse que je vois, c'est qu'il manque peut-être un noexcept specifier.

Une idée ?

Merci d'avance

[dalfab]

Bonjour,

Le warning parle de "template specialization" à éviter, alors que ta fonction n'est pas ça. C'est une surcharge de la template-fonction std::swap<>(), et toutes les surcharges dans ::std:: sont interdites (en C++03 c'était surement plus ambiguë).
Définir ta fonction swap() dans ton namespace data devrait être suffisant pour que ton swap() soit trouvé par ADL.
Il faudra peut-être vérifier que le code n'appelle pas std::swap(data::FixedPoint<>&,data::FixedPoint<>&) qui ne compilerait donc plus et les remplacer par :

Code :

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

1
2
using  ::std::swap; // penser à chercher dans la STL pour les types simples et les types de la STL (facultatif ici mais sinon permet de pouvoir tout 'swapper'
swap(...,...);       // 'swapper' en cherchant par A.D.L. donc dans le namespace des paramètres

[jo_link_noir]

https://en.cppreference.com/w/cpp/la.../extending_std

It is allowed to add template specializations for any standard library function template to the namespace std only if the declaration depends on at least one program-defined type and the specialization satisfies all requirements for the original template, except where such specializations are prohibited.
(until C++20)

It is undefined behavior to declare a full specialization of any standard library funtion template.
(since C++20)

https://en.cppreference.com/w/cpp/algorithm/swap

std::swap may be specialized in namespace std for program-defined types, but such specializations are not found by ADL (the namespace std is not the associated namespace for the program-defined type).
(until C++20)

En gros, mettre une spécialisation de swap dans le namespace std fonctionnera mal (et finalement de n'importe quelle fonction), car cela va dépendre de l'ordre des includes (une fonction n'utilise que les prototypes de fonction listé dans les lignes précédentes). D'où l'usage de l'ADL comme indiqué par @dalfab.

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
struct A{};
 
namespace std
{
  void swap(A&, A&) noexcept; 
}
 
#include <iostream>
 
namespace std
{
  void swap(A&, A&) noexcept { std::cout << "a\n"; }
}
 
class B{};
 
namespace std
{
  void swap(B&, B&) noexcept { std::cout << "b\n"; }
}
 
#include <algorithm>
 
int main()
{
  A a;
  std::iter_swap(&a, &a); // a
  std::swap(a,a); // a
 
  B b;
  std::iter_swap(&b, &b); // rien, swap(B&,B&) est déclarée après l'implémentation de std::iter_swap.
  std::swap(b,b); // b
}

(A et B ne sont pas template, mais le résultat est le même.)

[Bktero]

Le warning parle de "template specialization" à éviter, alors que ta fonction n'est pas ça. C'est une surcharge de la template-fonction std::swap<>(), et toutes les surcharges dans ::std:: sont interdites

Pourquoi c'est une surcharge et non une spécialisation ? J'aurais dit que c'était une spécialisation partielle, car il y a bien 2 arguments (qui sont template, certes).

It is undefined behavior to declare a full specialization of any standard library funtion template.
(since C++20)

J'avais vu cette subtilité. Donc ça veut que de toute façon, quand je vais passer à C++20, mon code sera forcément NOK.

Après avoir posté mon premier message, j'ai fait des essais et je me suis rendu compte que je pouvais supprimer ma spécialisation dans std:: et qu'appeler std::swap(FixedPoint<...>, FixedPoint<...>); fonctionnait toujours. La classe répond visiblement à certaines caractéristiques qui la rend swappable sans que j'ai rien à faire. Exemple :

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
#include <iostream>
#include <utility>
 
template<typename T>
struct FixedPoint {
	static_assert(std::is_integral_v<T>);
	T value;
};
 
static_assert(std::is_swappable_v<FixedPoint<int>>, "Foo is not swappable");
 
int main() {
	FixedPoint<int> a{1};
	FixedPoint<int> b{2};
 
	std::cout << a.value << '\n';
	std::cout << b.value << '\n';
 
	std::cout << "swap" << '\n';
 
	std::swap(a, b);
 
	std::cout << a.value << '\n';
	std::cout << b.value << '\n';
}

Sortie :

1
2
swap
2
1

Je suppose que je rentre dans le point (1) (je suis en C++17) :

1) Swaps the values a and b. This overload does not participate in overload resolution unless std::is_move_constructible_v<T> && std::is_move_assignable_v<T> is true. (since C++17)

En effet, ces quelques lignes compilent :

Code :

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

1
2
3
4
using T = FixedPoint<int>;
static_assert(std::is_move_constructible_v<T>);
static_assert(std::is_move_assignable_v<T>);
static_assert(std::is_swappable_v<T>);

J'ai essayé pour voir mais std::swap(a, b); fonctionne quand même en C++14 donc j'ai un doute...

Dans le cas où std::swap() ne fournirait pas directement, cette page dit ce qu'il faut faire en fait https://en.cppreference.com/w/cpp/named_req/Swappable Elle parle notamment de votre technique : déclarer une fonction swap(), libre dans le namespace de la classe ou friend à l'intérieur de la classe. La seconde solution a une subtilité avec l'ADL. Mais je ne suis pas du tout familier de l'ADL, il faut que je comprenne ce que c'est pour faire la différence entre les 2 solutions.

[dalfab]

Pourquoi c'est une surcharge et non une spécialisation ? J'aurais dit que c'était une spécialisation partielle, car il y a bien 2 arguments (qui sont template, certes).

C'est une surcharge, tu définis une fonction swap. Une spécialisation, ça défini une "version" de swap qui correspond à un cas particulier d'une fonction-template. Ça a le format : template<param de template ou vide> swap<ici les valeurs attendue par une fonction swap modèle>(paramètres compatibles avec la fonction-modèle) {.... Par exemple on peut spécialiser le swap de std::vector par

Code :

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

1
2
template<>
void swap<FixedPoint<int>,std::allocator<FixedPoint<int>>>(std::vector<FixedPoint<int>,std::allocator<FixedPoint<int>>>& _Left, std::vector<FixedPoint,std::allocator<FixedPoint>>& _Right) noexcept;

De plus une spécialisation partielle de classe c'est possible, mais une spécialisation partielle de fonction ça n'existe pas (à moins que j'aie loupé un épisode!), c'est toujours une totale.

J'avais vu cette subtilité. Donc ça veut que de toute façon, quand je vais passer à C++20, mon code sera forcément NOK.

Je l'apprends, au moins on est fixé, fini les "bidouilles".

Après avoir posté mon premier message, j'ai fait des essais et je me suis rendu compte que je pouvais supprimer ma spécialisation dans std:: et qu'appeler std::swap(FixedPoint<...>, FixedPoint<...>); fonctionnait toujours. La classe répond visiblement à certaines caractéristiques qui la rend swappable sans que j'ai rien à faire.

Je pense qu'à moins d'être tordu, la plupart des objets copiables sont swappables. L'intérêt de mettre sa propre fonction swap c'est surtout d'en écrire une version plus optimale.

Dans le cas où std::swap() ne fournirait pas directement, cette page dit ce qu'il faut faire en fait https://en.cppreference.com/w/cpp/named_req/Swappable Elle parle notamment de votre technique : déclarer une fonction swap(), libre dans le namespace de la classe ou friend à l'intérieur de la classe. La seconde solution a une subtilité avec l'ADL. Mais je ne suis pas du tout familier de l'ADL, il faut que je comprenne ce que c'est pour faire la différence entre les 2 solutions.

Les 2 méthodes sont compatibles avec l'ADL, mais la seconde crée une fonction qui n'est presque plus accessible sauf justement par ADL.
L'ADL en deux mots c'est : si on a une fonction (sans la préfixer d'aucun ::) ou un opérateur, on cherchera la fonction/opérateur prioritairement dans le namespace où sont les types de ses paramètres (même si ce namespace n'est pas demandé explicitement.)
Dans ton cas, comme ton objet est défini dans un namespace data, c'est dans ce namespace qu'il ira chercher une fonction swap. Si on met std::swap, ça stoppe l'ADL et la fonction est cherchée uniquement dans std.