From d5b05f428859b21f7499a8ad148cfa5be2921d5f Mon Sep 17 00:00:00 2001 From: frederic-tingaud-sonarsource Date: Tue, 8 Oct 2024 14:43:30 +0000 Subject: [PATCH 1/9] Create rule S7121 --- rules/S7121/cfamily/metadata.json | 25 ++++++++++++++++++ rules/S7121/cfamily/rule.adoc | 44 +++++++++++++++++++++++++++++++ rules/S7121/metadata.json | 2 ++ 3 files changed, 71 insertions(+) create mode 100644 rules/S7121/cfamily/metadata.json create mode 100644 rules/S7121/cfamily/rule.adoc create mode 100644 rules/S7121/metadata.json diff --git a/rules/S7121/cfamily/metadata.json b/rules/S7121/cfamily/metadata.json new file mode 100644 index 00000000000..f4adb2c54df --- /dev/null +++ b/rules/S7121/cfamily/metadata.json @@ -0,0 +1,25 @@ +{ + "title": "FIXME", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-7121", + "sqKey": "S7121", + "scope": "All", + "defaultQualityProfiles": ["Sonar way"], + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "HIGH", + "RELIABILITY": "MEDIUM", + "SECURITY": "LOW" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc new file mode 100644 index 00000000000..69c2a03a830 --- /dev/null +++ b/rules/S7121/cfamily/rule.adoc @@ -0,0 +1,44 @@ +FIXME: add a description + +// If you want to factorize the description uncomment the following line and create the file. +//include::../description.adoc[] + +== Why is this an issue? + +FIXME: remove the unused optional headers (that are commented out) + +//=== What is the potential impact? + +== How to fix it +//== How to fix it in FRAMEWORK NAME + +=== Code examples + +==== Noncompliant code example + +[source,cpp,diff-id=1,diff-type=noncompliant] +---- +FIXME +---- + +==== Compliant solution + +[source,cpp,diff-id=1,diff-type=compliant] +---- +FIXME +---- + +//=== How does this work? + +//=== Pitfalls + +//=== Going the extra mile + + +//== Resources +//=== Documentation +//=== Articles & blog posts +//=== Conference presentations +//=== Standards +//=== External coding guidelines +//=== Benchmarks diff --git a/rules/S7121/metadata.json b/rules/S7121/metadata.json new file mode 100644 index 00000000000..2c63c085104 --- /dev/null +++ b/rules/S7121/metadata.json @@ -0,0 +1,2 @@ +{ +} From 3ca54d3c2c057842dc1ca80f1c1e5a52aac46e35 Mon Sep 17 00:00:00 2001 From: Fred Tingaud Date: Tue, 8 Oct 2024 18:41:27 +0200 Subject: [PATCH 2/9] WIP --- rules/S7121/cfamily/rule.adoc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 69c2a03a830..74f4326b6ad 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -9,6 +9,21 @@ FIXME: remove the unused optional headers (that are commented out) //=== What is the potential impact? +=== Exception + +This rule does not raise when explicitly creating a string from `c_str()`. + +[source,cpp] +---- +void takeString(std::string const& dest); + +void func(std::string const& src) { + takeString(std::string(src.c_str())); // Compliant by exception +} +---- + +This operation could be done on purpose in the rare case where `src` is a string containing null-terminators and we want to only keep the content up to the first null-terminator in `dst`. + == How to fix it //== How to fix it in FRAMEWORK NAME From 78ba39583bb2fde8ea9d3c679b0fe3b9cb75d89b Mon Sep 17 00:00:00 2001 From: Fred Tingaud Date: Fri, 11 Oct 2024 15:56:25 +0200 Subject: [PATCH 3/9] Rule description --- rules/S7121/cfamily/metadata.json | 10 +++--- rules/S7121/cfamily/rule.adoc | 51 ++++++++++++------------------- 2 files changed, 24 insertions(+), 37 deletions(-) diff --git a/rules/S7121/cfamily/metadata.json b/rules/S7121/cfamily/metadata.json index f4adb2c54df..ba18c65f1f0 100644 --- a/rules/S7121/cfamily/metadata.json +++ b/rules/S7121/cfamily/metadata.json @@ -1,5 +1,5 @@ { - "title": "FIXME", + "title": "Redundant calls to c_str() should be avoided", "type": "CODE_SMELL", "status": "ready", "remediation": { @@ -13,13 +13,11 @@ "sqKey": "S7121", "scope": "All", "defaultQualityProfiles": ["Sonar way"], - "quickfix": "unknown", + "quickfix": "targeted", "code": { "impacts": { - "MAINTAINABILITY": "HIGH", - "RELIABILITY": "MEDIUM", - "SECURITY": "LOW" + "MAINTAINABILITY": "MEDIUM" }, - "attribute": "CONVENTIONAL" + "attribute": "EFFICIENT" } } diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 74f4326b6ad..49463c332d7 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -1,54 +1,43 @@ -FIXME: add a description - -// If you want to factorize the description uncomment the following line and create the file. -//include::../description.adoc[] +Unnecessary calls to `c_str()` or `data()` reduce readability and performances. == Why is this an issue? -FIXME: remove the unused optional headers (that are commented out) +In the process of refactoring code from C to C++, it can easily happen that redundant calls to `c_str()` or `data()` on standard strings get introduced when the string could be used directly. -//=== What is the potential impact? +[source,cpp,diff-id=1,diff-type=noncompliant] +---- +void takeString(std::string const& dest); -=== Exception +void func(std::string const& src) { + takeString(src.c_str()); // Noncompliant: redundant copy +} +---- -This rule does not raise when explicitly creating a string from `c_str()`. +Not only is this unnecessary step a readability issue, but it will force the creation of uselesse intermediary strings , reducing the performances. It should be removed. -[source,cpp] +[source,cpp,diff-id=1,diff-type=compliant] ---- void takeString(std::string const& dest); void func(std::string const& src) { - takeString(std::string(src.c_str())); // Compliant by exception + takeString(src); // Compliant } ---- -This operation could be done on purpose in the rare case where `src` is a string containing null-terminators and we want to only keep the content up to the first null-terminator in `dst`. - -== How to fix it -//== How to fix it in FRAMEWORK NAME - -=== Code examples +=== Exception -==== Noncompliant code example +This rule does not raise when explicitly creating a string from `c_str()`. -[source,cpp,diff-id=1,diff-type=noncompliant] ----- -FIXME +[source,cpp] ---- +void takeString(std::string const& dest); -==== Compliant solution - -[source,cpp,diff-id=1,diff-type=compliant] ----- -FIXME +void func(std::string const& src) { + takeString(std::string(src.c_str())); // Compliant by exception +} ---- -//=== How does this work? - -//=== Pitfalls - -//=== Going the extra mile - +This operation could be done on purpose in the rare case where `src` is a string containing null-terminators and we want to only keep the content up to the first null-terminator in `dst`. //== Resources //=== Documentation From 0d6634bd70b43f5ee86f8b1b7eec7e0f7164a64e Mon Sep 17 00:00:00 2001 From: Fred Tingaud <95592999+frederic-tingaud-sonarsource@users.noreply.github.com> Date: Fri, 11 Oct 2024 15:58:33 +0200 Subject: [PATCH 4/9] Update rule.adoc --- rules/S7121/cfamily/rule.adoc | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 49463c332d7..62891188c52 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -1,4 +1,4 @@ -Unnecessary calls to `c_str()` or `data()` reduce readability and performances. +Unnecessary calls to `c_str()` or `data()` reduce readability and performance. == Why is this an issue? @@ -13,7 +13,7 @@ void func(std::string const& src) { } ---- -Not only is this unnecessary step a readability issue, but it will force the creation of uselesse intermediary strings , reducing the performances. It should be removed. +Not only is this unnecessary step a readability issue, but it will force the creation of useless intermediary strings, reducing the performance. [source,cpp,diff-id=1,diff-type=compliant] ---- @@ -37,12 +37,5 @@ void func(std::string const& src) { } ---- -This operation could be done on purpose in the rare case where `src` is a string containing null-terminators and we want to only keep the content up to the first null-terminator in `dst`. +This operation could be done on purpose in the rare case where `src` is a string containing embedded null-terminators, in order to only keep the content up to the first null-terminator in `dest`. -//== Resources -//=== Documentation -//=== Articles & blog posts -//=== Conference presentations -//=== Standards -//=== External coding guidelines -//=== Benchmarks From f2a254b61e17382336fd80a6f201c5e2332c86b5 Mon Sep 17 00:00:00 2001 From: Fred Tingaud Date: Tue, 15 Oct 2024 12:15:21 +0200 Subject: [PATCH 5/9] Apply suggestions --- rules/S7121/cfamily/metadata.json | 2 +- rules/S7121/cfamily/rule.adoc | 26 +++++++++++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/rules/S7121/cfamily/metadata.json b/rules/S7121/cfamily/metadata.json index ba18c65f1f0..8cd899d13c9 100644 --- a/rules/S7121/cfamily/metadata.json +++ b/rules/S7121/cfamily/metadata.json @@ -1,5 +1,5 @@ { - "title": "Redundant calls to c_str() should be avoided", + "title": "Calls to c_str() should not implicitly recreate strings or string_views", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 62891188c52..541aad37cbe 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -2,31 +2,42 @@ Unnecessary calls to `c_str()` or `data()` reduce readability and performance. == Why is this an issue? -In the process of refactoring code from C to C++, it can easily happen that redundant calls to `c_str()` or `data()` on standard strings get introduced when the string could be used directly. +In the process of refactoring code from C to C++, it can easily happen that redundant calls to `c_str()` +or `data()` on standard strings or string_views get introduced when the original string could be used directly. [source,cpp,diff-id=1,diff-type=noncompliant] ---- void takeString(std::string const& dest); -void func(std::string const& src) { +void takeStringView(std::string_view dest); + +void func(std::string const& src, std::string_view srcView) { takeString(src.c_str()); // Noncompliant: redundant copy + takeStringView(src.c_str()); // Noncompliant: redundant copy + takeStringView(srcView.data()); // Noncompliant: redundant crossing of the content } ---- -Not only is this unnecessary step a readability issue, but it will force the creation of useless intermediary strings, reducing the performance. +Not only is this unnecessary step a readability issue, but it will force the creation of useless intermediary strings, +or force to cross the content of a string_view to search for the null-terminator, reducing the performance. +The call to `c_str()` or `data()` should be removed. [source,cpp,diff-id=1,diff-type=compliant] ---- void takeString(std::string const& dest); -void func(std::string const& src) { +void takeStringView(std::string_view dest); + +void func(std::string const& src, std::string_view srcView) { takeString(src); // Compliant + takeStringView(src); // Compliant + takeStringView(srcView); // Compliant } ---- === Exception -This rule does not raise when explicitly creating a string from `c_str()`. +This rule does not raise when explicitly creating a string or a string_view from `c_str()`. [source,cpp] ---- @@ -39,3 +50,8 @@ void func(std::string const& src) { This operation could be done on purpose in the rare case where `src` is a string containing embedded null-terminators, in order to only keep the content up to the first null-terminator in `dest`. +== Resources + +=== Related rules + +* S6231 - "std::string_view" and "std::span" parameters should be directly constructed from sequences From 7ea481eadfaca9ab62d8eaf98a817d43135fde48 Mon Sep 17 00:00:00 2001 From: Tomasz Kaminski Date: Mon, 21 Oct 2024 10:26:16 +0200 Subject: [PATCH 6/9] Removed std::string_view source --- rules/S7121/cfamily/rule.adoc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 541aad37cbe..80f891a3857 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -3,7 +3,7 @@ Unnecessary calls to `c_str()` or `data()` reduce readability and performance. == Why is this an issue? In the process of refactoring code from C to C++, it can easily happen that redundant calls to `c_str()` -or `data()` on standard strings or string_views get introduced when the original string could be used directly. +or `data()` on standard strings get introduced when the original string could be used directly. [source,cpp,diff-id=1,diff-type=noncompliant] ---- @@ -11,15 +11,15 @@ void takeString(std::string const& dest); void takeStringView(std::string_view dest); -void func(std::string const& src, std::string_view srcView) { +void func(std::string const& src) { takeString(src.c_str()); // Noncompliant: redundant copy - takeStringView(src.c_str()); // Noncompliant: redundant copy - takeStringView(srcView.data()); // Noncompliant: redundant crossing of the content + takeStringView(src.c_str()); // Noncompliant: redundant crossing of the content + takeStringView(src.data()); // Noncompliant: redundant crossing of the content } ---- Not only is this unnecessary step a readability issue, but it will force the creation of useless intermediary strings, -or force to cross the content of a string_view to search for the null-terminator, reducing the performance. +and force to cross the content of a string to search for the null-terminator, reducing the performance. The call to `c_str()` or `data()` should be removed. [source,cpp,diff-id=1,diff-type=compliant] @@ -28,10 +28,10 @@ void takeString(std::string const& dest); void takeStringView(std::string_view dest); -void func(std::string const& src, std::string_view srcView) { +void func(std::string const& src) { takeString(src); // Compliant takeStringView(src); // Compliant - takeStringView(srcView); // Compliant + takeStringView(src); // Compliant } ---- From 2f2e13b3535ce7c0e5bfbde5d300493f7709ad69 Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Mon, 21 Oct 2024 11:41:56 +0200 Subject: [PATCH 7/9] Apply suggestions from code review --- rules/S7121/cfamily/rule.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 80f891a3857..5bd46bb310f 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -3,7 +3,7 @@ Unnecessary calls to `c_str()` or `data()` reduce readability and performance. == Why is this an issue? In the process of refactoring code from C to C++, it can easily happen that redundant calls to `c_str()` -or `data()` on standard strings get introduced when the original string could be used directly. +or `data()` on standard strings get introduced when the original object could be used directly. [source,cpp,diff-id=1,diff-type=noncompliant] ---- @@ -19,7 +19,7 @@ void func(std::string const& src) { ---- Not only is this unnecessary step a readability issue, but it will force the creation of useless intermediary strings, -and force to cross the content of a string to search for the null-terminator, reducing the performance. +or force to cross the content of a string to search for the null-terminator, reducing the performance. The call to `c_str()` or `data()` should be removed. [source,cpp,diff-id=1,diff-type=compliant] From 60c23e2dab0c927c69e88d6fa3419a1d0ca5f72a Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Mon, 28 Oct 2024 08:02:35 +0100 Subject: [PATCH 8/9] Changed quick-fix to covered --- rules/S7121/cfamily/metadata.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/S7121/cfamily/metadata.json b/rules/S7121/cfamily/metadata.json index 8cd899d13c9..5fee60b374b 100644 --- a/rules/S7121/cfamily/metadata.json +++ b/rules/S7121/cfamily/metadata.json @@ -13,7 +13,7 @@ "sqKey": "S7121", "scope": "All", "defaultQualityProfiles": ["Sonar way"], - "quickfix": "targeted", + "quickfix": "covered", "code": { "impacts": { "MAINTAINABILITY": "MEDIUM" From 606235308f1d4af1616ca664ad4182554a464fba Mon Sep 17 00:00:00 2001 From: Fred Tingaud <95592999+frederic-tingaud-sonarsource@users.noreply.github.com> Date: Wed, 30 Oct 2024 18:04:37 +0100 Subject: [PATCH 9/9] Update rules/S7121/cfamily/rule.adoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Loïc Joly --- rules/S7121/cfamily/rule.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/S7121/cfamily/rule.adoc b/rules/S7121/cfamily/rule.adoc index 5bd46bb310f..19c9ab934a4 100644 --- a/rules/S7121/cfamily/rule.adoc +++ b/rules/S7121/cfamily/rule.adoc @@ -19,7 +19,7 @@ void func(std::string const& src) { ---- Not only is this unnecessary step a readability issue, but it will force the creation of useless intermediary strings, -or force to cross the content of a string to search for the null-terminator, reducing the performance. +or require searching the content of a string for the null-terminator, reducing the performance. The call to `c_str()` or `data()` should be removed. [source,cpp,diff-id=1,diff-type=compliant]