| Author |
 Topic  |
|
|
FriendlyRabbit
Senior Member
Germany
28 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. |
Edited by - FriendlyRabbit on Mar 21 2025 07:14:06 AM |
|
|
feline
Whole Tomato Software
United Kingdom
19147 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
Senior Member
Germany
28 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
19147 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
Senior Member
Germany
28 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
19147 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 |
|
|
|
FriendlyRabbit
Senior Member
Germany
28 Posts |
Posted - Mar 20 2025 : 05:57:36 AM
|
| I just noticed that this feature request is almost 18 months old. Are there any news? |
|
|
|
feline
Whole Tomato Software
United Kingdom
19147 Posts |
Posted - Mar 20 2025 : 07:35:09 AM
|
I am assuming you mean case=150079, the case for configuring the "Detect magic numbers" code inspection.
This is actually marked as having been implemented and released back in VA 2506.0, but somehow this thread wasn't updated.
Select the "Detect magic numbers" check in the VA Options dialog, and click on the cog icon to display the configuration options for this checker. |
zen is the art of being at one with the two'ness |
|
|
|
FriendlyRabbit
Senior Member
Germany
28 Posts |
Posted - Mar 20 2025 : 07:58:09 AM
|
Yes, I have already noticed. I was talking about the alias detection. That would be really nice to have.
By the way, are you aware of how often new checks are added to clang-tidy? I wonder if this still happens or if they have saturated. Are new checks by default enabled or disabled in VisualAssist? My preferred way is to have all checks enabled by default and then I disable the ones I do not want. |
|
|
|
feline
Whole Tomato Software
United Kingdom
19147 Posts |
Posted - Mar 20 2025 : 08:42:05 AM
|
Ah, I guessed wrong. case=150074 is still open, no updates yet on that.
From our release notes, VA 2491 updated clang-tidy to version 16.0. I don't know the details, but from the bug comments, a certain amount of code editing is required before we can compile clang into VA. So it's not as simple as just dropping in a clang update and having it applied for "free".
This list of checks, and the first few checks I looked at:
https://clang.llvm.org/extra/clang-tidy/checks/list.html
none of them are mentioning when a given check was added to clang-tidy. The information is no doubt somewhere, but I don't know how quickly the list of checks is growing. I assume it is growing slowly with time, but there are already quite a lot of checks, so I doubt it is growing that fast. |
zen is the art of being at one with the two'ness |
|
|
|
FriendlyRabbit
Senior Member
Germany
28 Posts |
Posted - Mar 21 2025 : 07:13:24 AM
|
Good to hear it's still on your agenda.
Another question: Does having aliases enabled have an impact on performance? Having a lot of checks enabled adds up to a significant delay, so I wonder if it is worth to find and disable all aliases of enabled checks.
In other words: If I enable both modernize-avoid-c-arrays and cppcoreguidelines-avoid-c-arrays, is the check executed once or twice? |
|
|
|
feline
Whole Tomato Software
United Kingdom
19147 Posts |
Posted - Mar 24 2025 : 2:07:23 PM
|
How large are your code files, and how many results are being returned?
I have tried a test function, designed to pick up the array check several times over. This function has been duplicated 500 times in a single file, giving me a 14,000 line file. Even in this file, Code Inspection is finishing in a couple of seconds. It simply isn't taking long enough for turning on or off "cppcoreguidelines-avoid-c-arrays" while I already have "modernize-avoid-c-arrays" turned On to make any reliable difference in the time taken.
Perhaps this is still to small and simple a test, but if so, what sort of scale are your files? |
zen is the art of being at one with the two'ness |
|
|
|
feline
Whole Tomato Software
United Kingdom
19147 Posts |
Posted - Mar 25 2025 : 09:56:58 AM
|
Thinking about this from a different angle, I have set:
cppcoreguidelines-avoid-c-arrays = level 3
modernize-avoid-c-arrays = level 4
so they are marked with different colours, and set up a small simple test that is being picked up by these checks. The array lines are only ever returned once, and if I only have one check active, that is the colour that is used to mark these issues in the code file. But with both active they are shown with the level 3 underlining colour. |
zen is the art of being at one with the two'ness |
|
|
|
FriendlyRabbit
Senior Member
Germany
28 Posts |
Posted - Mar 26 2025 : 03:29:46 AM
|
Just to make it clear, it is not about these checks in particular. I just mentioned them as an example of two checks with different name but identical behavior. Executing them both is a waste of resources, and I wonder if Visual Assist detects this.
I have many files with 800-1000 lines, and I am waiting around 10-20 seconds for the code inspections to finish. I think it is not the size of the files but the huge number of checks I have enabled. I have almost all of them enabled, which is more than 400. That is why I believe that disabling the approximately 70 duplicates could boost performance.
I would not mind the checks to run in parallel to speed things up. Is this already done? Maybe doing this by default is a bit greedy, but having a "Check now" button which processes the checks as fast as possible and with a progress bar sounds good to me.
I can imagine there are simple and fast checks and also complex and slow checks. It could even be that a single check takes most of the time. I have no feedback on this. |
|
|
|
feline
Whole Tomato Software
United Kingdom
19147 Posts |
Posted - Mar 26 2025 : 10:09:49 AM
|
Honestly I don't know how the different checks are being passed to clang-tidy, if this is done as separate calls, or one very long call. Even assuming this is done as one long call, I don't know how clang-tidy is structured internally. It is tempting to assume the checks would run in parallel, but given they can be checking very different things, I am not sure that would make much sense.
As for the duplicates, why not simply uncheck the duplicates in the VA options dialog? Yes, it will take a little while to find and unselect one of each, but the settings should be quite safe.
I have set up a simple test here, with a different selection of checks installed, starting with VA 2515, and then upgraded to and opened each of the following releases. So 7 updates to new builds, and my customized list of Code Inspection checks has been preserved at every step. So you only need to do this once.
Plus, for extra security, you can export your current VA settings, which includes your Code Inspection settings, via:
VA Options -> Performance -> Export Settings
giving you a reg file to backup. |
zen is the art of being at one with the two'ness |
|
|
| |
Topic |
|
readability-magic-numbers and alias detection
