Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create rule S7172 Named methods should be used to avoid confusion between testing an optional or an expected and testing the wrapped value CPP-5929 #4545

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 28, 2024

You can preview this rule here (updated a few minutes after each push).

CPP-5929

@github-actions github-actions bot added the cfamily C / C++ / Objective-C label Nov 28, 2024
Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@loic-joly-sonarsource loic-joly-sonarsource changed the title Create rule S7172 Create rule S7172 Named methods should be used to avoid confusion between testing an optional or an expected and testing the wrapped value Dec 19, 2024
@loic-joly-sonarsource loic-joly-sonarsource changed the title Create rule S7172 Named methods should be used to avoid confusion between testing an optional or an expected and testing the wrapped value Create rule S7172 Named methods should be used to avoid confusion between testing an optional or an expected and testing the wrapped value CPP-5929 Dec 19, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A genral suggestion, I would use "contained value" and "wrapper", so there is no so many "wrapper" words in one code block.

rules/S7172/cfamily/rule.adoc Outdated Show resolved Hide resolved
rules/S7172/cfamily/rule.adoc Outdated Show resolved Hide resolved
rules/S7172/cfamily/rule.adoc Outdated Show resolved Hide resolved
FIXME: add a description

// If you want to factorize the description uncomment the following line and create the file.
//include::../description.adoc[]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a line here:

This rule raises an issue when std::optional, boost::optional, or std::expected wrap a basic type, and the conversion to bool is used to test presence of the value.

In SonarLint it is just displayed below the title and before the tabs, saying while this is an issue. So it is good place, to give tldr of the rule

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one additional suggestion.

Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cfamily C / C++ / Objective-C
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants