| Author |
 Topic  |
|
|
FriendlyRabbit
Junior Member
Germany
20 Posts |
 Posted - Sep 25 2023 : 02:47:41 AM
|
The code inspection readability-magic-numbers complains about 0.5, 0.25 and 10 to be magic numbers. I would like to configure these numbers as non-magic because they are common in my code, and defining const double onehalf = 0.5; does not make the code easier to read IMHO.
I noticed that the clang-tidy checker does allow some configuration, especially IgnoredIntegerValues and IgnoredFloatingPointValues. Basically I want just that. Can you please add this?
https://clang.llvm.org/extra/clang-tidy/checks/readability/magic-numbers.html
As a side question: Is there any penalty in enabling readability-magic-numbers AND cppcoreguidelines-avoid-magic-numbers? Since they are just aliases for each other, I hope only one is executed at runtime, so that I don't have to manually check all aliases. |
|
|
feline
Whole Tomato Software
United Kingdom
18915 Posts |
Posted - Sep 25 2023 : 11:00:02 AM
|
These two checks only show up if you have turned On "Show Unevaluated Checkers". I suspect VA is simply asking clang-tidy for a list of all checks, which is where the unevaluated checkers list is coming from. I wasn't actually aware there were any aliases, so I have put in a feature request to filter all of the aliases out of this list, since these should only be shown once:
case=150074
Since the magic number check is still an unevaluated checker we don't yet expose any settings the check supports.
So first I would only turn on one version of this check, since if you turn on both versions I suspect (but don't know for a fact) that VA will be calling the check twice, since we don't know it is an alias.
If you don't want these numbers to be flagged, do you want to keep this check turned on at all? I generally like to move all magic numbers into variables, since experience teaches they have to be updated sooner or later, but if it is a number you are sure will never change then it makes sense to leave them alone.
Do you want VA to check some magic numbers but not others? Are there a lot that you want to have VA ignore while still checking for others? If so you can skip VA checkers in your code, on a case by case or file by file basis, as explained in the "Disabling Code Inspections for Specific Code Sections" section here:
https://docs.wholetomato.com/default.asp?W760
for example, assuming you are only using the "readability-magic-numbers" code inspection, it is skipped on the magic number in this function:
double AvoidMagicNumbers(double dInputValue)
{
return dInputValue * 0.348; // vaCI:skip:readability-magic-numbers
}
|
zen is the art of being at one with the two'ness |
 |
|
|
FriendlyRabbit
Junior Member
Germany
20 Posts |
Posted - Sep 26 2023 : 02:59:38 AM
|
There are many very useful unevaluated checkers, that is why I enabled all of them and then disabled the ones which are not that useful.
Since I am new here, please tell me how I can use the case number. Is there something like Jira where I can see the open issues?
Detecting and filtering duplicates is a very good idea. You can find the full list of aliases here.
https://clang.llvm.org/extra/clang-tidy/checks/list.html
I noticed that if I enable both readability-magic-numbers AND cppcoreguidelines-avoid-magic-numbers, only one writes results in the code inspection result list. Not sure what's happening there.
I disagree about them being shown only once. I image people expect to find the code inspection with both the original and the alias name, since the connection is not always as obvious as with magic numbers, e.g. fuchsia-header-anon-namespaces and google-build-namespaces are the same thing. Or maybe a team agrees on using all cppcoreguideline checks, then the alias cppcoreguidelines-explicit-virtual-functions must be in the list or people will forget to enable it. I guess you need to somehow visualize the aliases in the list and maybe synchronize the enabled/disabled property between a code inspection and its alias(es).
Yes, I definitely want this check because often a named constant is indeed better. I just need some allowed exceptions like 0.5 or 0.25. Being able to disable a check for one line is nice (thank you for the link), but it makes the code even longer than just naming the constant ;-)
|
Edited by - FriendlyRabbit on Sep 26 2023 03:01:44 AM |
|
|
|
feline
Whole Tomato Software
United Kingdom
18915 Posts |
Posted - Sep 26 2023 : 12:56:54 PM
|
Unfortunately our case database isn't publicly visible, but we do aim to update threads here when a bug or feature request is done, so if you monitor the forum, or check the release notes, you will have an idea of what is happening. Plus you can always ask for an update here.
You are right about the aliases, any thoughts on how this should best be handled? I hadn't considered that, but yes, people may well be looking for the check under either the old or new name.
For the magic numbers, would skipping the check for specific files work well for now? You can do this easily enough, via the comment:
// vaCI:skipall:readability-magic-numbers
at the top of the file.
I have put in a feature request to move this check from the unevaluated checkers, and to also support at least some of its options, so you can have lists of numbers to ignore:
case=150079 |
zen is the art of being at one with the two'ness |
|
|
|
FriendlyRabbit
Junior Member
Germany
20 Posts |
Posted - Sep 27 2023 : 02:33:34 AM
|
Don't worry about the magic numbers issue for now. I will rather keep them enabled as a reminder to review the code later.
I can only make suggestions on how to handle the aliases. I see the following options
1. Show them as independent code inspections but keep their configuration (enabled/disabled, other options) synchronized. So if I enable fuchsia-header-anon-namespaces, google-build-namespaces becomes enabled as well. That might become a bit unintuitive, however. People might enable cppcoreguidelines and then disable all google checks for some reason, then they would by accident disable some cppcoreguidelines as well as they are aliases.
2. In the code inspection list, merge aliases together as a single code inspection and write all names on the right (currently written in grey next to the checkbox). That would make each check findable with its own name. The description must be picked from the original check since the alias description is usually just its name or "This is an alias for ...".
I prefer solution 2. If one thing has several names, show it as one thing with several names. That looks like a clean solution to me. |
|
|
|
feline
Whole Tomato Software
United Kingdom
18915 Posts |
Posted - Sep 27 2023 : 06:49:40 AM
|
Solution 2 makes the most sense to me as well, I have put a note on the case about this.
Magic numbers is a feature request, and we are slowly adding more of the clang-tidy checks to the fully supported list, so it is good to be aware that the options for the magic number check matter. |
zen is the art of being at one with the two'ness |
|
|
| |
Topic |
|
|
|
Configuration of readability-magic-numbers
