5

PVS-Studio, the static code analyzer, for the following bit of code

size_t const n = 4;
int a[n] = {};

reports:

V112 Dangerous magic number 4 used: ...t const n = 4;. test.cpp 3

Although PVS-Studio is used with Visual Studio 2017 project and reports the same warning for both, 32 and 64 bit, those build configurations are not taken into account by the analyzer, AFAIU.

I would have expected the context to be analysed better and treat the code above as equivalent to this

int a[4] = {};

for which PVS-Studio does not issue any diagnostics.

In the case above is this dangerous magic number N used, a false positive?

What are the reasons the two code samples above are not analyzed as equivalent?

mloskot
  • 37,086
  • 11
  • 109
  • 136
  • 8
    If anything I would assume the second example's use of magic numbers is less safe than the first one, making it more worthy of a warning. It seems that the warning is related to `n = 4` and not with the use of `n` as an array size. – François Andrieux Jul 19 '17 at 15:42
  • I'm personally not very impressed with the quality of PVS Studios analysis, and this result is in line with what I expect from it - low quality warning - not all that useful. – Jesper Juhl Jul 19 '17 at 15:45
  • 5
    It is all explained in the link you got from PVS-Studio and posted. Even why `int a[4]` doesn't get warning. – Marek Vitek Jul 19 '17 at 15:47
  • @MarekVitek Unless the page has been updated over the last 48h, it does neither explain specifically *why* the former does and why the latter doesn't issue the warning. Mind you, the last paragraph is not satisfactory explanation. – mloskot Jul 19 '17 at 16:05
  • @FrançoisAndrieux Yes, indeed. – mloskot Jul 19 '17 at 16:06
  • @JesperJuhl I don't want to turn this QA into review of PVS-Studio. FWIW, I'm not entirely satisfied with VS2017 analyzer myself, for instance. – mloskot Jul 19 '17 at 16:06
  • My understanding is that the analyzer is complaining about the number `4`. As an experiment, try "#define ARRAY_CAPACITY (4U)`, instead of `const size_t n = 4;`. You may need to configure the analyzer for C++. – Thomas Matthews Jul 19 '17 at 16:15
  • @ThomasMatthews Preprocessor macro is 1:1 equivalent to the second case `a[4]`. Complains about number `4` is clear, but how on earth one is supposed to use constant/literal without putting there the `4` ;-) – mloskot Jul 19 '17 at 16:22
  • 2
    @mloskot Number 4 is mentioned in the table as one of the numbers that might be causing troubles when porting to 64 bit and require special attention. `int a[4]` is mentioned as special case that gets exempted from the check. Use 5 as your constant and there will be no warning. – Marek Vitek Jul 19 '17 at 16:23
  • 1
    Or better yet [suppress this one instance](https://www.viva64.com/en/m/0017/#ID0ELH) add `//-V112` after line where warning is issued. – Marek Vitek Jul 19 '17 at 16:27
  • 1
    The static analyzer may have rules requiring that numbers other than 0 or 1 be a macro; regardless of the equivalency to the second case. – Thomas Matthews Jul 19 '17 at 16:27
  • @MarekVitek Is that seriously and true? I have read the table but as reader, I took "Let's list magic numbers" as "let's list all magic numbers we can see in the examples above", but not as "let's list PVS-specific magic numbers". I suspect, most readers would understand it that way, no? I clearly didn't pay enough attention to the table content besides the `4` or `0xffffffff` :-) FYI, `n=4` comes from nanodbc library tests, what a coincidence! – mloskot Jul 19 '17 at 16:27
  • 1
    Is that seriously and true? Yes. The magic numbers (for detect 64-bit issues) is 4, 32, 0x7FFFFFFF, 0x80000000 and 0xFFFFFFFF. Details: https://www.viva64.com/en/l/0009/ – AndreyKarpov Jul 19 '17 at 16:32
  • @AndreyKarpov OK. It would be helpful if this article is linked from the docs though. MarekVitek thanks for digging this up! – mloskot Jul 19 '17 at 16:34
  • 1
    @mloskot Yes it is seriously true. These numbers have special meaning in lot of code hence the special attention to it. The reason is because in many cases their meaning changes with transition to 64 bit system. – Marek Vitek Jul 19 '17 at 16:46
  • @MarekVitek I stand corrected. I haven't paid enough attention reading the docs page, apparently. Thanks – mloskot Jul 19 '17 at 16:56
  • 1
    @mloskot Well you are right in one point. The doc page could use bit of rework. Move the table to the top would be first step. And little bit change of wording might help too. But I am not an user of that tool. – Marek Vitek Jul 19 '17 at 17:06

3 Answers3

3

This

size_t const n = 4;
int a[n] = {};

is false positive.

64-bit diagnostics are very noisy and there is nothing you can do about it. Yes, the analyzer produces many false positives such as magic numbers like 4, 0xFFFFFFFF, etc. In the analyzer a lot of exceptions has already been made when it doesn’t complain (for example: int a[4] = {};). However, there are still so many options of using constants that it’s impossible to foresee all of them.

When porting code to 64-bit system it makes sense to look through all the magic numbers, to make sure that the programmer, for example, does not expect that the pointer size is 4 bytes somewhere. Then it makes sense to switch off V112 diagnostic so that it does not bother you.

mloskot
  • 37,086
  • 11
  • 109
  • 136
AndreyKarpov
  • 1,083
  • 6
  • 17
  • Where did you come up with 64-bit? I didn't see it in the example lines. This could be on a 32-bit or 16-bit system. – Thomas Matthews Jul 19 '17 at 16:16
  • I have updated my question to clarify I use PVS with VS2017 – mloskot Jul 19 '17 at 16:21
  • > "Where did you come up with 64-bit?" This diagnostic was created for detect 64-bit issues. https://www.viva64.com/en/l/0009/ – AndreyKarpov Jul 19 '17 at 16:24
  • @AndreyKarpov Can you relate to the comments by MarekVitek above? If that is correct, may I suggest to re-phrase the docs as "Let's list specific magic numbers that PVS-Studio is especially concerned about..." or something alike – mloskot Jul 19 '17 at 16:31
  • 1
    I did not understand the question. The magic numbers (for detect 64-bit issues) is 4, 32, 0x7FFFFFFF, 0x80000000 and 0xFFFFFFFF. – AndreyKarpov Jul 19 '17 at 16:36
2

Reading the link you posted, I concluded it is a false positive in your case.

The tool is assuming you are going to use n in a malloc (or equivalent allocation procedure) statement to be equivalent to the size of int (or any 4 bytes variable). So the recommendation is to use sizeof(desired type).

If you were using n inside a malloc statement, it would make sense - since int (or any other type) could vary for different architectures (if not now, in the future). But apparently this is not your case.

Leonardo Alves Machado
  • 2,747
  • 10
  • 38
  • 53
  • How do you get the assumption of using malloc? The OP is declaring an automatic variable, not using `malloc` or `new`. The constant identifier could be used in assignment or arithmetic expressions. – Thomas Matthews Jul 19 '17 at 16:17
  • 1
    As I said, my conclusion was based on what I have read on the link posted on the question: https://www.viva64.com/en/w/V112/print/ - It uses `malloc` as example to explain it. – Leonardo Alves Machado Jul 19 '17 at 16:20
  • @ThomasMatthews Because PVS-Studio apparently doesn't go on deep analysis where the variable is used. It sees const being assigned 4 and issues warning. But maybe I am wrong. – Marek Vitek Jul 19 '17 at 16:56
2

Number 4 is considered as one of the potentially dangerous numbers when porting from 32 bit to 64 bit hence warning on const being assigned 4. Other numbers are listed in the table behind the link you posted. With examples how it can be dangerous.

You can suppress individual warning by adding //-V112 at the end of line you are sure is 100% OK.

size_t const n = 4; //-V112

This will suppreess the warning and you can focus on your work again.

As for int a[4] = {}; PVS-Studio considers it a special case for which it doesn't issue warning. Why it doesn't take it into account in the first case I don't know. But it looks like hardcoded exception for really specific case.

If you are not building 64 bit builds, then I assume it is safe to disable the warning alltogether for now. But be warned - comes from sight comes from mind.

Marek Vitek
  • 1,573
  • 9
  • 20