Modify rule S6147: refactor directory structure, set quickfix and code-snippet language
This commit is contained in:
Arseniy Zaostrovnykh
GitHub
parent
d0757e5066
commit
0b9b390141
@ -1,3 +1,35 @@
|
||||
{
|
||||
|
||||
"title": "Use discriminated unions or \"std::variant\"",
|
||||
"type": "CODE_SMELL",
|
||||
"code": {
|
||||
"impacts": {
|
||||
"MAINTAINABILITY": "HIGH"
|
||||
},
|
||||
"attribute": "COMPLETE"
|
||||
},
|
||||
"status": "ready",
|
||||
"remediation": {
|
||||
"func": "Constant\/Issue",
|
||||
"constantCost": "20min"
|
||||
},
|
||||
"tags": [
|
||||
"clumsy",
|
||||
"pitfall"
|
||||
],
|
||||
"extra": {
|
||||
"replacementRules": [
|
||||
|
||||
],
|
||||
"legacyKeys": [
|
||||
|
||||
]
|
||||
},
|
||||
"defaultSeverity": "Critical",
|
||||
"ruleSpecification": "RSPEC-6147",
|
||||
"sqKey": "S6147",
|
||||
"scope": "All",
|
||||
"defaultQualityProfiles": [
|
||||
"Sonar way"
|
||||
],
|
||||
"quickfix": "infeasible"
|
||||
}
|
||||
|
||||
@ -1 +1,110 @@
|
||||
include::../rule.adoc[]
|
||||
== Why is this an issue?
|
||||
|
||||
In order to save memory, unions allow you to use the same memory to store objects from a list of possible types as long as one object is stored at a time. Unions are not inherently safe, as they expect you to externally keep track of the type of value they currently hold.
|
||||
|
||||
|
||||
Wrong tracking has the potential to corrupt memory or to trigger undefined behaviors.
|
||||
|
||||
|
||||
A straightforward way to avoid it is storing the information about the currently active alternative along with the union. Here follow suggested patterns to do that:
|
||||
|
||||
|
||||
[source, cpp]
|
||||
----
|
||||
typedef int altType1;
|
||||
typedef float altType2;
|
||||
|
||||
// Pattern 1
|
||||
union alternativesCommonStartingFieldPattern {
|
||||
struct {
|
||||
bool isAlt1;
|
||||
altType1 a1;
|
||||
} one;
|
||||
|
||||
struct {
|
||||
bool isAlt1;
|
||||
altType2 a2;
|
||||
} two;
|
||||
};
|
||||
|
||||
double getValueAsDouble(alternativesCommonStartingFieldPattern *pattern1) {
|
||||
return pattern1->one.isAlt1?pattern1->one.a1:pattern1->two.a2;
|
||||
}
|
||||
----
|
||||
|
||||
This pattern uses the fact that when two alternatives of a standard layout union are standard-layout-structs that share a common initial sequence, it is allowed to read this common initial sequence on one alternative even if the other alternative is the one currently active. This is commonly used to limit the number of bits required to store the discriminant.
|
||||
|
||||
|
||||
[source, cpp]
|
||||
----
|
||||
// Pattern 2
|
||||
struct wrappedUnionPattern {
|
||||
enum {ALTTYPE1, ALTTYPE2} type;
|
||||
|
||||
union {
|
||||
altType1 a1;
|
||||
altType2 a2;
|
||||
};
|
||||
};
|
||||
|
||||
double getValueAsDouble(wrappedUnionPattern *pattern2) {
|
||||
return (pattern2->type==wrappedUnionPattern::ALTTYPE1)?pattern2->a1:pattern2->a2;
|
||||
}
|
||||
----
|
||||
This pattern is more straightforward, and wraps the union inside a structure that will also store the discriminant. Note that in this case, the union itself can be anonymous.
|
||||
|
||||
[source, cpp]
|
||||
----
|
||||
// Pattern 3 (C++17)
|
||||
using stdVariantPattern = std::variant<altType1, altType2>;
|
||||
|
||||
double getValueAsDouble(stdVariantPattern *pattern3) {
|
||||
return std::visit([](auto&& alternative) -> double { return alternative;}, *pattern3);
|
||||
}
|
||||
----
|
||||
This pattern relies on {cpp}17’s ``++std::variant++`` to store the alternative.
|
||||
|
||||
|
||||
In general, ``++std::variant++`` is:
|
||||
|
||||
* Safer as the type of the current value is always known and checked before usage.
|
||||
* More practical as it can have members of any type, including non trivial types (see S6025). It also supports redundant types, which is useful when alternatives have the same type with different semantic meanings.
|
||||
* Easier to use as it provides many member/helper functions.
|
||||
|
||||
One noticeable difference with unions is that the alternatives in a ``++std::variant++`` do not have a name. You can access them by type or by index, using ``++std::get++`` (throws if the wrong alternative is accessed) or ``++std::get_if++`` (returns a null pointer if the wrong alternative is used). But very often, instead of accessing a specific alternative, visitors are used to distinguish cases of the variant.
|
||||
|
||||
|
||||
This rule raises an issue when unions are used outside of the 3 suggested patterns.
|
||||
|
||||
|
||||
=== Noncompliant code example
|
||||
|
||||
[source,cpp,diff-id=1,diff-type=noncompliant]
|
||||
----
|
||||
void rawUnion() {
|
||||
union IntOrDouble { // Noncompliant: union is not wrapped
|
||||
int i;
|
||||
double d;
|
||||
};
|
||||
IntOrDouble intOrDouble;
|
||||
intOrDouble.d = 10.5;
|
||||
}
|
||||
----
|
||||
|
||||
|
||||
=== Compliant solution
|
||||
|
||||
[source,cpp,diff-id=1,diff-type=compliant]
|
||||
----
|
||||
struct IntOrChar {
|
||||
enum { INT, CHAR } tag;
|
||||
union { // Compliant
|
||||
int i;
|
||||
char c;
|
||||
};
|
||||
};
|
||||
|
||||
void simpleVariant() {
|
||||
std::variant<int, double> intOrDouble = 10.5; // Compliant
|
||||
}{code}
|
||||
----
|
||||
|
||||
@ -1,35 +1,2 @@
|
||||
{
|
||||
"title": "Use discriminated unions or \"std::variant\"",
|
||||
"type": "CODE_SMELL",
|
||||
"code": {
|
||||
"impacts": {
|
||||
"MAINTAINABILITY": "HIGH"
|
||||
},
|
||||
"attribute": "COMPLETE"
|
||||
},
|
||||
"status": "ready",
|
||||
"remediation": {
|
||||
"func": "Constant\/Issue",
|
||||
"constantCost": "20min"
|
||||
},
|
||||
"tags": [
|
||||
"clumsy",
|
||||
"pitfall"
|
||||
],
|
||||
"extra": {
|
||||
"replacementRules": [
|
||||
|
||||
],
|
||||
"legacyKeys": [
|
||||
|
||||
]
|
||||
},
|
||||
"defaultSeverity": "Critical",
|
||||
"ruleSpecification": "RSPEC-6147",
|
||||
"sqKey": "S6147",
|
||||
"scope": "All",
|
||||
"defaultQualityProfiles": [
|
||||
"Sonar way"
|
||||
],
|
||||
"quickfix": "unknown"
|
||||
}
|
||||
|
||||
@ -1,113 +0,0 @@
|
||||
== Why is this an issue?
|
||||
|
||||
In order to save memory, unions allow you to use the same memory to store objects from a list of possible types as long as one object is stored at a time. Unions are not inherently safe, as they expect you to externally keep track of the type of value they currently hold.
|
||||
|
||||
|
||||
|
||||
Wrong tracking has the potential to corrupt memory or to trigger undefined behaviors.
|
||||
|
||||
|
||||
A straightforward way to avoid it is storing the information about the currently active alternative along with the union. Here follow suggested patterns to do that:
|
||||
|
||||
|
||||
|
||||
|
||||
----
|
||||
typedef int altType1;
|
||||
typedef float altType2;
|
||||
|
||||
// Pattern 1
|
||||
union alternativesCommonStartingFieldPattern {
|
||||
struct {
|
||||
bool isAlt1;
|
||||
altType1 a1;
|
||||
} one;
|
||||
|
||||
struct {
|
||||
bool isAlt1;
|
||||
altType2 a2;
|
||||
} two;
|
||||
};
|
||||
|
||||
double getValueAsDouble(alternativesCommonStartingFieldPattern *pattern1) {
|
||||
return pattern1->one.isAlt1?pattern1->one.a1:pattern1->two.a2;
|
||||
}
|
||||
----
|
||||
This pattern uses the fact that when two alternatives of a standard layout union are standard-layout-structs that share a common initial sequence, it is allowed to read this common initial sequence on one alternative even if the other alternative is the one currently active. This is commonly used to limit the number of bits required to store the discriminant.
|
||||
|
||||
|
||||
|
||||
|
||||
----
|
||||
// Pattern 2
|
||||
struct wrappedUnionPattern {
|
||||
enum {ALTTYPE1, ALTTYPE2} type;
|
||||
|
||||
union {
|
||||
altType1 a1;
|
||||
altType2 a2;
|
||||
};
|
||||
};
|
||||
|
||||
double getValueAsDouble(wrappedUnionPattern *pattern2) {
|
||||
return (pattern2->type==wrappedUnionPattern::ALTTYPE1)?pattern2->a1:pattern2->a2;
|
||||
}
|
||||
----
|
||||
This pattern is more straightforward, and wraps the union inside a structure that will also store the discriminant. Note that in this case, the union itself can be anonymous.
|
||||
|
||||
----
|
||||
// Pattern 3 (C++17)
|
||||
using stdVariantPattern = std::variant<altType1, altType2>;
|
||||
|
||||
double getValueAsDouble(stdVariantPattern *pattern3) {
|
||||
return std::visit([](auto&& alternative) -> double { return alternative;}, *pattern3);
|
||||
}
|
||||
----
|
||||
This pattern relies on {cpp}17’s ``++std::variant++`` to store the alternative.
|
||||
|
||||
|
||||
In general, ``++std::variant++`` is:
|
||||
|
||||
* Safer as the type of the current value is always known and checked before usage.
|
||||
* More practical as it can have members of any type, including non trivial types (see S6025). It also supports redundant types, which is useful when alternatives have the same type with different semantic meanings.
|
||||
* Easier to use as it provides many member/helper functions.
|
||||
|
||||
One noticeable difference with unions is that the alternatives in a ``++std::variant++`` do not have a name. You can access them by type or by index, using ``++std::get++`` (throws if the wrong alternative is accessed) or ``++std::get_if++`` (returns a null pointer if the wrong alternative is used). But very often, instead of accessing a specific alternative, visitors are used to distinguish cases of the variant.
|
||||
|
||||
|
||||
This rule raises an issue when unions are used outside of the 3 suggested patterns.
|
||||
|
||||
|
||||
=== Noncompliant code example
|
||||
|
||||
[source,text]
|
||||
----
|
||||
void rawUnion() {
|
||||
union IntOrDouble { // Noncompliant: union is not wrapped
|
||||
int i;
|
||||
double d;
|
||||
};
|
||||
IntOrDouble intOrDouble;
|
||||
intOrDouble.d = 10.5;
|
||||
}
|
||||
----
|
||||
|
||||
|
||||
=== Compliant solution
|
||||
|
||||
[source,text]
|
||||
----
|
||||
struct IntOrChar {
|
||||
enum { INT, CHAR } tag;
|
||||
union { // Compliant
|
||||
int i;
|
||||
char c;
|
||||
};
|
||||
};
|
||||
|
||||
void simpleVariant() {
|
||||
std::variant<int, double> intOrDouble = 10.5; // Compliant
|
||||
}{code}
|
||||
|
||||
----
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user