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

Remove enforcement rules of rule F.17 #2162

Open
pauljansen42 opened this issue Dec 17, 2023 · 7 comments
Open

Remove enforcement rules of rule F.17 #2162

pauljansen42 opened this issue Dec 17, 2023 · 7 comments

Comments

@pauljansen42
Copy link
Contributor

Rule F.17 (For “in-out” parameters, pass by reference to non-const) and Con.3 (By default, pass pointers and references to consts) have both two enforcement rules of which one of them is nearly identical:

  • F.17: Warn about functions regarding reference to non-const parameters that do not write to them
  • Con.3 Flag a function that does not modify an object passed by pointer or reference to non-const

This means that 2 rules will fire if a non-const reference parameter is not changed in the corresponding function body. Having 2 rules firing for the same situation is a coding standard design smell.

In order to solve this, I suggest to remove all enforcements from rule F.17 because the rule is about expresssing an intention, i.e. an in-out parameter. That is something you can't check automatically. To take this one step further, one might even consider to remove F.17 completely because we also have rule F.20 (For “out” output values, prefer return values to output parameters). Since F.20 describes some exceptions, removing F.17 might a bit too strong. So I suggest to remove the 2 enforcement rules of rule F.17.

@BenjamenMeyer
Copy link

In this case, the two rules are addressing different aspects. F rules are specifically about functions while Con rules are specifically about constants. The two compliment each other - with Con.3 being the general case and F.17 being a specific case of Con.3 within the function context.

@pauljansen42
Copy link
Contributor Author

Thanks for your answer Benjamen. As far as I can determine, F.17 is a full subset of Con.3. So every time F.17 fires, Con.3 will fire as well. In such a situation, the question is what the added value is of F.17 and from a coding standard point of view, F.17 should be removed. If F.17 is not a full subset of some other rule, it is important to extract its unique part and remove the overlap.

Is this something that can be done?

These questions/this issue might sound a bit pedantic, but we are dealing with customers using our implementation of your C++ core guidelines (we are talking about 50,000+ software engineers) that don't understand why a situation is flagged twice by different rules and I think they are right.

@iglesias
Copy link
Contributor

iglesias commented Jan 8, 2024

I think that the rules being categorized is understandable. Therefore, when a piece of code violates different categories - as it occurs in the described case with functions and constants categories - it is sensible that a rule in each category is triggered.

What is the underlying issue that their code cannot be fixed complying with const correctness?

@pauljansen42
Copy link
Contributor Author

Thanks for your comments Fernando! The problem is not that the users don't know how to fix violations of these rules but they get 2 violations about the same thing. One for Con.3 and one for F.17. This is hard to explain and means that rules overlap. In my opinion, rules should be as atomic as possible.

@iglesias
Copy link
Contributor

iglesias commented Jan 9, 2024

You're welcome Paul! I suppose with users now you're referring to the same 50,000+ software engineers mentioned earlier. That they don't know how to fix them is a problem, indeed, and, I believe, you're targeting at the wrong solution to this problem. Just in case it may help them, I reiterate, the violations are fixed by complying with const correctness.

@pauljansen42
Copy link
Contributor Author

I stated "The problem is NOT that the users don't know how to fix violations"...

@BenjamenMeyer
Copy link

@pauljansen42 I don't think its as much of an issue you think. Perhaps explain to them that while the categories are complimentary they're different categories which will also help point them to the issues and provide some clarity on why it got triggered where it did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants