How to correct the code segment?????????????

[biplab]

for(i=0;i<256;i++)
{
hist[i]=0;
}
for(i=0;i<height;i++)
{
for(j=0;j<width;j++)
{
k=(long int)rgb1[i][j];
hist[k]=hist[k]+1;
}
}

for(i=0;i<256;i++)
{
if(hist[i]!=0)
tmp++;
else continue;
}

sum_hist=(int**)malloc(tmp*sizeof(int*));
for(i=0;i<tmp;i++)
sum_hist[i]=(int*)malloc(2*sizeof(int));

for(i=0;i<256;i++)
{
if(hist[i]!=0)
{
//printf("%d %d\n",i,hist[i]);
sum_hist[i][0]=(int)i;
sum_hist[i][1]=(int)hist[i];
printf("%d %d\n",sum_hist[i][0],sum_hist[i][1]);

}
else continue;
}
for(i=0;i<tmp;i++)
printf("%d\n",sum_hist[i][1]);

In the above code segment sum_hist is a double pointer of type long
int...the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

[Andrew Poelstra]

On 2008-09-13, biplab <ge*******@gmail.comwrote:

for(i=0;i<256;i++)
{
hist[i]=0;
}

You need to fix your formatting. 2-space indents are
recommended for posting on Usenet. In any case, be
consistent, and use whitespace around operators.

Also, this could be replaced with:
memset(hist, 0, 256);

Or potentially:
memset(hist, 0, sizeof hist);

for(i=0;i<height;i++)
{
for(j=0;j<width;j++)
{
k=(long int)rgb1[i][j];

Why this cast? The way you use k here, all its logical
values could fit into an unsigned char. Surely then
whatever type it already was, is sufficient?

hist[k]=hist[k]+1;
}
}

for(i=0;i<256;i++)
{
if(hist[i]!=0)
tmp++;
else continue;

The else line is superfluous. Fix your formatting.

}

sum_hist=(int**)malloc(tmp*sizeof(int*));

Don't cast the return of malloc(). Don't use the return of
malloc() without checking it for NULL. Don't pass types to
sizeof.

for(i=0;i<tmp;i++)
sum_hist[i]=(int*)malloc(2*sizeof(int));

Ditto.

for(i=0;i<256;i++)
{
if(hist[i]!=0)
{
//printf("%d %d\n",i,hist[i]);

Don't use single-line comments on usenet, especially when
you are using a random and excessive indentation scheme.

sum_hist[i][0]=(int)i;
sum_hist[i][1]=(int)hist[i];

What's with these casts?

printf("%d %d\n",sum_hist[i][0],sum_hist[i][1]);

}
else continue;

This line is superfluous.

}
for(i=0;i<tmp;i++)
printf("%d\n",sum_hist[i][1]);

In the above code segment sum_hist is a double pointer of type long
int...the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

Fix the above issues and post a compilable "segment" of code. Then
we will see what the problem is.

--
Andrew Poelstra ap*******@wpsoftware.com
To email me, use the above email addresss with .com set to .net

[Andrew Poelstra]

On 2008-09-13, biplab <ge*******@gmail.comwrote:

>
<code snipped>

In the above code segment sum_hist is a double pointer of type long
int...the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

Here is your code, corrected for the (mainly stylistic) issues I
mentioned else thread. It is abundantly clear to me now what the
problem is. I hope it is also clear to you:

/* Begin uncompilable segment */

memset(hist, 0, 256);

for(i = 0; i < height; i++)
for(j = 0; j < width; j++)
{
k = rgb1[i][j];
if(hist[k] == 0)
++count;
++hist[k];
}

sum_hist = malloc(count * sizeof *sum_hist);
for(i = 0; i < count; i++)
sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

for(i = 0; i < 256; i++)
if(hist[i] != 0)
{
/* printf("%d %d\n", i, hist[i]); */
sum_hist[i][0] = i;
sum_hist[i][1] = hist[i];
printf("%d %d\n", sum_hist[i][0], sum_hist[i][1]);
}

for(i = 0; i < count; i++)
printf("%d\n", sum_hist[i][1]);

/* End segment */

--
Andrew Poelstra ap*******@wpsoftware.com
To email me, use the above email addresss with .com set to .net

[Andrew Poelstra]

On 2008-09-13, Andrew Poelstra <ap*******@supernova.homewrote:

On 2008-09-13, biplab <ge*******@gmail.comwrote:

>>
<code snipped>

In the above code segment sum_hist is a double pointer of type long
int...the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

Here is your code, corrected for the (mainly stylistic) issues I
mentioned else thread. It is abundantly clear to me now what the
problem is. I hope it is also clear to you:

/* Begin uncompilable segment */

memset(hist, 0, 256);

for(i = 0; i < height; i++)
for(j = 0; j < width; j++)
{
k = rgb1[i][j];
if(hist[k] == 0)
++count;
++hist[k];
}

sum_hist = malloc(count * sizeof *sum_hist);
for(i = 0; i < count; i++)
sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

I forgot to add checks that malloc() does not return NULL.

for(i = 0; i < 256; i++)
if(hist[i] != 0)
{
/* printf("%d %d\n", i, hist[i]); */
sum_hist[i][0] = i;

To the OP, what happens when i exceeds count?

sum_hist[i][1] = hist[i];
printf("%d %d\n", sum_hist[i][0], sum_hist[i][1]);
}

for(i = 0; i < count; i++)
printf("%d\n", sum_hist[i][1]);

/* End segment */

--
Andrew Poelstra ap*******@wpsoftware.com
To email me, use the above email addresss with .com set to .net

[Richard]

Andrew Poelstra <ap*******@supernova.homewrites:

On 2008-09-13, biplab <ge*******@gmail.comwrote:

>>
<code snipped>

In the above code segment sum_hist is a double pointer of type long
int...the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

Here is your code, corrected for the (mainly stylistic) issues I
mentioned else thread. It is abundantly clear to me now what the
problem is. I hope it is also clear to you:

/* Begin uncompilable segment */

memset(hist, 0, 256);

Horrible. Code fails review.

>
for(i = 0; i < height; i++)
for(j = 0; j < width; j++)
{
k = rgb1[i][j];
if(hist[k] == 0)
++count;
++hist[k];
}

sum_hist = malloc(count * sizeof *sum_hist);
for(i = 0; i < count; i++)
sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

Silly to use sizeof on a variable element. Overly clever and liable to
mislead. It also assumes optimization will spot the obvious.

>
for(i = 0; i < 256; i++)

minus another point for hard coding numbers.

if(hist[i] != 0)
{
/* printf("%d %d\n", i, hist[i]); */
sum_hist[i][0] = i;
sum_hist[i][1] = hist[i];
printf("%d %d\n", sum_hist[i][0], sum_hist[i][1]);
}

for(i = 0; i < count; i++)
printf("%d\n", sum_hist[i][1]);

/* End segment */

The whole thing is weak. It wreaks of needing a structure in place
anyway.

[Richard Tobin]

In article <ga**********@registered.motzarella.org>,
Richard <rg****@gmail.comwrote:

> sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

>Silly to use sizeof on a variable element. Overly clever and liable to
mislead.

I don't see why. How would you be misled?

>It also assumes optimization will spot the obvious.

What optimisation are you talking about? The result of sizeof is (in
the absence of variable-length arrays) a constant, and the constant
will be just as constant as if you had written, say, the type name.
It will surely have been determined before code generation.

-- Richard
--
Please remember to mention me / in tapes you leave behind.

[Richard]

ri*****@cogsci.ed.ac.uk (Richard Tobin) writes:

In article <ga**********@registered.motzarella.org>,
Richard <rg****@gmail.comwrote:

>> sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

>>Silly to use sizeof on a variable element. Overly clever and liable to
mislead.

I don't see why. How would you be misled?

It suggests to the naive programmer that there are variable sized
elements. Of course there can not be. We know that. The reader should
know that. But why state it in this way? Just "silly". Not the worst in
the world I agree. But stylewise I personally do not like it.

>

>>It also assumes optimization will spot the obvious.

What optimisation are you talking about? The result of sizeof is (in
the absence of variable-length arrays) a constant, and the constant
will be just as constant as if you had written, say, the type name.
It will surely have been determined before code generation.

One would hope so. But its not a feat of genius to simply make it struct
and use a static sizeof.

*shrug* More a style thing. Can you be sure that

2 * sizeof h[i]

is compiled to the same as

2 * sizeof h[0]

for example? Pretty much yes, but why introducde confusion., Struct that
sucker anyway.

sizeof(elementStruct)

Simple.

>
-- Richard

--

[Keith Thompson]

Richard<rg****@gmail.comwrites:

Andrew Poelstra <ap*******@supernova.homewrites:

[...]

>Here is your code, corrected for the (mainly stylistic) issues I
mentioned else thread. It is abundantly clear to me now what the
problem is. I hope it is also clear to you:

/* Begin uncompilable segment */

memset(hist, 0, 256);

Horrible. Code fails review.

Did you have something in mind other than the use of the magic number
256?

>for(i = 0; i < height; i++)
for(j = 0; j < width; j++)
{
k = rgb1[i][j];
if(hist[k] == 0)
++count;
++hist[k];
}

sum_hist = malloc(count * sizeof *sum_hist);
for(i = 0; i < count; i++)
sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

Silly to use sizeof on a variable element. Overly clever and liable to
mislead. It also assumes optimization will spot the obvious.

It's simply an instance of the usual clc idiom for calling malloc:
ptr = malloc(count * sizeof *ptr);
with "sum_hist[i]" replacing "ptr". It won't mislead anyone who's
familiar with the idiom. As for assuming that "optimization will spot
the obvious", I'm afraid I have no idea what you mean; would you care
to explain? (If you're referring to not evaluating the operand of
sizeof, that's not an optimization, it's a language requirement.)

[snip]

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[Keith Thompson]

biplab <ge*******@gmail.comwrites:

for(i=0;i<256;i++)
{
hist[i]=0;
}
for(i=0;i<height;i++)
{
for(j=0;j<width;j++)
{
k=(long int)rgb1[i][j];
hist[k]=hist[k]+1;
}
}

for(i=0;i<256;i++)
{
if(hist[i]!=0)
tmp++;
else continue;
}

sum_hist=(int**)malloc(tmp*sizeof(int*));
for(i=0;i<tmp;i++)
sum_hist[i]=(int*)malloc(2*sizeof(int));

for(i=0;i<256;i++)
{
if(hist[i]!=0)
{
//printf("%d %d\n",i,hist[i]);
sum_hist[i][0]=(int)i;
sum_hist[i][1]=(int)hist[i];
printf("%d %d\n",sum_hist[i][0],sum_hist[i][1]);

}
else continue;
}
for(i=0;i<tmp;i++)
printf("%d\n",sum_hist[i][1]);

In the above code segment sum_hist is a double pointer of type long
int...

Don't tell us, show us. In other words, post a complete compilable
program. You haven't shown us *any* declarations. Don't make us
guess how everything is declared.

I assume you mean that num_hist is of type long int**. Please don't
refer to a pointer-to-pointer type as a "double pointer"; that refers
to a pointer to type double.

the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

An ellipsis...i.e., a sequence of three dots...doesn't mean...what
you...think it...means.

If you use more nearly standard punctuation (a sentence ends with a
single period, the first letter of a sentence is capitalized), it will
be easier for us to read what you write, and therefore easier for us
to help you. We don't insist on flawless spelling or grammar, but
please make some effort.

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[Keith Thompson]

Richard<rg****@gmail.comwrites:

ri*****@cogsci.ed.ac.uk (Richard Tobin) writes:

>In article <ga**********@registered.motzarella.org>,
Richard <rg****@gmail.comwrote:

>>> sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

>>>Silly to use sizeof on a variable element. Overly clever and liable to
mislead.

I don't see why. How would you be misled?

It suggests to the naive programmer that there are variable sized
elements. Of course there can not be. We know that. The reader should
know that. But why state it in this way? Just "silly". Not the worst in
the world I agree. But stylewise I personally do not like it.

Perhaps naive progammers would benefit from the learning experience.

>>>It also assumes optimization will spot the obvious.

What optimisation are you talking about? The result of sizeof is (in
the absence of variable-length arrays) a constant, and the constant
will be just as constant as if you had written, say, the type name.
It will surely have been determined before code generation.

One would hope so. But its not a feat of genius to simply make it struct
and use a static sizeof.

So there is no "optimization" involved.

*shrug* More a style thing. Can you be sure that

2 * sizeof h[i]

is compiled to the same as

2 * sizeof h[0]

for example?

You can't be sure that it will compile to the same code, but you can
be certain it will have the same effect (assuming a non-buggy
compiler). And evaluating the operand of sizeof is the kind of bug
that I'd expect to be caught long before the compiler goes out the
door. You might as well worry about whether sizeof(char)==1.

Pretty much yes, but why introducde confusion., Struct that
sucker anyway.

sizeof(elementStruct)

Simple.

Ok, so sum_hist is an array of elementStruct (apparently a typedef;
you didn't mention that), and

sum_hist[i] = malloc(2 * sizeof(elementStruct));

does the right thing. Until later, during program maintenance, when
sum_hist is changed so it's an array of some_other_type. Your version
continues to compile without complaint. If some_other_type is bigger
than elementStruct, you're wasting space; if it's slightly smaller,
it's possible that the error won't even be detected until you're
demonstrating the code for an important customer.

Or you can just write:

sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

and when the type of sum_hist is changed, the code will continue to
compile *and* to work properly (though I might want a declared
constant rather than 2). It's not a universally known idiom, but it's
easy to understand once you know the purpose.

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[Richard]

Keith Thompson <ks***@mib.orgwrites:

Richard<rg****@gmail.comwrites:

>Andrew Poelstra <ap*******@supernova.homewrites:

[...]

>>Here is your code, corrected for the (mainly stylistic) issues I
mentioned else thread. It is abundantly clear to me now what the
problem is. I hope it is also clear to you:

/* Begin uncompilable segment */

memset(hist, 0, 256);

Horrible. Code fails review.

Did you have something in mind other than the use of the magic number
256?

Nope.

>

>>for(i = 0; i < height; i++)
for(j = 0; j < width; j++)
{
k = rgb1[i][j];
if(hist[k] == 0)
++count;
++hist[k];
}

sum_hist = malloc(count * sizeof *sum_hist);
for(i = 0; i < count; i++)
sum_hist[i] = malloc(2 * sizeof *sum_hist[i]);

Silly to use sizeof on a variable element. Overly clever and liable to
mislead. It also assumes optimization will spot the obvious.

It's simply an instance of the usual clc idiom for calling malloc:

I know what it is. And what form of it it is.

ptr = malloc(count * sizeof *ptr);
with "sum_hist[i]" replacing "ptr". It won't mislead anyone who's
familiar with the idiom. As for assuming that "optimization will spot
the obvious", I'm afraid I have no idea what you mean; would you care
to explain? (If you're referring to not evaluating the operand of
sizeof, that's not an optimization, it's a language requirement.)

[snip]

--

[Barry Schwarz]

On Sat, 13 Sep 2008 14:45:49 GMT, Andrew Poelstra
<ap*******@supernova.homewrote:

>On 2008-09-13, biplab <ge*******@gmail.comwrote:

>for(i=0;i<256;i++)
{
hist[i]=0;
}

You need to fix your formatting. 2-space indents are
recommended for posting on Usenet. In any case, be
consistent, and use whitespace around operators.

Also, this could be replaced with:
memset(hist, 0, 256);

Only if sizeof hist[i] is 1 which is unlikely for anything other than
a char.

>
Or potentially:
memset(hist, 0, sizeof hist);

Why was the mistake prone recommendation above a definite and this
which looks correct a potential?
--
Remove del for email

[Barry Schwarz]

On Sat, 13 Sep 2008 07:17:08 -0700 (PDT), biplab <ge*******@gmail.com>
wrote:

>for(i=0;i<256;i++)
{
hist[i]=0;
}
for(i=0;i<height;i++)
{
for(j=0;j<width;j++)
{
k=(long int)rgb1[i][j];
hist[k]=hist[k]+1;
}
}

for(i=0;i<256;i++)
{
if(hist[i]!=0)
tmp++;
else continue;
}

sum_hist=(int**)malloc(tmp*sizeof(int*));

Since you claim below that sum_hist is a long**, this may not allocate
the correct amount of space if sizeof(int*) < sizeof(long*).

In any event, this code requires a diagnostic because there is no
implicit converstion between int** and long**.

> for(i=0;i<tmp;i++)
sum_hist[i]=(int*)malloc(2*sizeof(int));

This code is even more likely to fail since sizeof(long) is more
frequently greater than sizeof(int). The same diagnostic is required
regarding the implicit pointer conversion.

>
for(i=0;i<256;i++)
{
if(hist[i]!=0)
{
//printf("%d %d\n",i,hist[i]);
sum_hist[i][0]=(int)i;
sum_hist[i][1]=(int)hist[i];
printf("%d %d\n",sum_hist[i][0],sum_hist[i][1]);

%d requires an int. You are passing a long. This invokes undefined
behavior.

>
}
else continue;
}
for(i=0;i<tmp;i++)
printf("%d\n",sum_hist[i][1]);

In the above code segment sum_hist is a double pointer of type long
int...the problem is that when the values of sum_hist is printed withn
the is block..it is giving correct result..but when outside the for
loop..the value is printed......wrong values are printed.....how to
correct the code...

Post a compilable example that demonstrated the undesired behavior.

--
Remove del for email

[Andrew Poelstra]

On 2008-09-14, Barry Schwarz <sc******@dqel.comwrote:

On Sat, 13 Sep 2008 14:45:49 GMT, Andrew Poelstra
<ap*******@supernova.homewrote:

>>On 2008-09-13, biplab <ge*******@gmail.comwrote:

>>for(i=0;i<256;i++)
{
hist[i]=0;
}

You need to fix your formatting. 2-space indents are
recommended for posting on Usenet. In any case, be
consistent, and use whitespace around operators.

Also, this could be replaced with:
memset(hist, 0, 256);

Only if sizeof hist[i] is 1 which is unlikely for anything other than
a char.

Of course. My bad.

>>
Or potentially:
memset(hist, 0, sizeof hist);

Why was the mistake prone recommendation above a definite and this
which looks correct a potential?

The original code did not declare any variables (hence my
mistake above) and I was unsure if hist was an array or a
pointer.

--
Andrew Poelstra ap*******@wpsoftware.com
To email me, use the above email addresss with .com set to .net