Sign in to follow this  
Geometrian

C++ Static Code Analysis Failure

Recommended Posts

Geometrian    1810
Hi,

This is the exact C++ code in question:[CODE]threads = new glLibInternal_MetaWorker*[glLibInternal_numof_threads];
for (int i=0;i<glLibInternal_numof_threads;++i) threads[i] = new glLibInternal_MetaWorker(this,mesh);
int i = 0;
for (int x=0;x<grid_res[0];++x) { for (int y=0;y<grid_res[1];++y) { for (int z=0;z<grid_res[2];++z) {
threads[i]->owned_coords.push_back(x);
threads[i]->owned_coords.push_back(y);
threads[i]->owned_coords.push_back(z);
++i;
if (i==glLibInternal_numof_threads) i = 0;
}}}[/CODE]The variable glLibInternal_numof_threads is an external global int that, at runtime, is set to 12. grid_res[3] is an int array, with all elements equal to 16.

As general good practice, I ran this through static code analysis. Visual Studio gives (on the fifth line in the above source):
warning C6385: Invalid data: accessing 'threads', the readable size is 'int glLib::glLibInternal_numof_threads*4' bytes, but '16' bytes might be read

My question is how? Am I missing something obvious?

Thanks,
Ian Edited by Geometrian

Share this post


Link to post
Share on other sites
ApochPiQ    23000
Probably because of the shadowing of the variable "i". Shadowing is generally a bad idea and can lead to subtle bugs. I suggest naming the outer loop counter something different, or alternatively not using "i" as the name of the secondary counter.


Or, more likely: what's the type of "threads"? Edited by ApochPiQ

Share this post


Link to post
Share on other sites
l0calh05t    1796
True. There's no shadowing here. The only potential problem i see here is when glLibInternal_numof_threads is zero. In that case the second loop (in which the warning is triggered) would still access threads[0], despite there being no threads. Also, static analyzers aren't perfect, so maybe the static analyzer is just confused by the way wraparound is handled for i. Maybe try >= instead of ==. Edited by l0calh05t

Share this post


Link to post
Share on other sites
ApochPiQ    23000
[quote name='RulerOfNothing' timestamp='1339315695' post='4947849']
What outer loop? The loop that uses 'i' as a counter is only one line long.
[/quote]

Heh, good catch. That code is bloody hard to read.

Share this post


Link to post
Share on other sites
mhagain    13430
[quote name='ApochPiQ' timestamp='1339358628' post='4948004']
[quote name='RulerOfNothing' timestamp='1339315695' post='4947849']
What outer loop? The loop that uses 'i' as a counter is only one line long.
[/quote]

Heh, good catch. That code is bloody hard to read.
[/quote]
I can only second this - code analysis and a compiler will do so much, but to a human being reading it, it just looks [i]wrong[/i].

Share this post


Link to post
Share on other sites
mhagain    13430
Even some whitespace helps - try this for more readable:[code]threads = new glLibInternal_MetaWorker *[glLibInternal_numof_threads];

for (int i = 0; i < glLibInternal_numof_threads; ++i)
threads[i] = new glLibInternal_MetaWorker (this, mesh);

int i = 0;

for (int x = 0; x < grid_res[0]; ++x)
{
for (int y = 0; y < grid_res[1]; ++y)
{
for (int z = 0; z < grid_res[2]; ++z)
{
threads[i]->owned_coords.push_back (x);
threads[i]->owned_coords.push_back (y);
threads[i]->owned_coords.push_back (z);

++i;

if (i == glLibInternal_numof_threads) i = 0;
}
}
}[/code]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this