Sign in to follow this  
Geometrian

C++ Static Code Analysis Failure

Recommended Posts

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
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
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
[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
[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
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