Couple points, I'll try to make them brief.
On Dec 6, 2013, at 4:20 PM, [email protected] wrote:
> --- brlcad/trunk/src/rt/heatgraph.c 2013-12-06 21:18:17 UTC (rev 58859)
> +++ brlcad/trunk/src/rt/heatgraph.c 2013-12-06 21:20:14 UTC (rev 58860)
> @@ -147,9 +147,9 @@
> if (timeTable == NULL) {
> bu_log("X is %d, Y is %d\n", x, y);
> bu_log("Making time Table\n");
> - timeTable = bu_malloc(x * sizeof(double *), "timeTable");
> + timeTable = (fastf_t **)bu_malloc(x * sizeof(double *),
> "timeTable");
> for (i = 0; i < x; i++) {
> - timeTable[i] = bu_malloc(y * sizeof(double), "timeTable[i]");
> + timeTable[i] = (fastf_t *)bu_malloc(y * sizeof(double),
> "timeTable[i]");
It's always a big red flag when an *alloc() sizeof type does not match the
return cast type. That was also observed in the prior commit that r58863
fixed. Indeed, those above are wrong too. It looks like it should probably be
allocating sizeof fastf_t's, which I just fixed and committed.
Point is only that we need to be extra careful going forward with C++
compilation changes, especially where casts are concerned because it's a very
strong declaration to the C compiler. Once or twice is an oops, we move on.
More than that, it's time to reflect and adjust. Quelling warnings in general
is a high-risk source for introducing bugs so I think we just need to openly
recognize them when we encounter them so there in the thought forefront.
Casting *alloc() returns actually makes me cringe a bit for C, but hopefully
the other benefits will outweigh that particular wart. A lot of the other
issues (like propagating constness) I've seen look good.
On Dec 6, 2013, at 11:05 AM, [email protected] wrote:
> Revision: 58851
> http://sourceforge.net/p/brlcad/code/58851
> Author: tbrowder2
> Date: 2013-12-06 16:05:23 +0000 (Fri, 06 Dec 2013)
> Log Message:
> -----------
> various fixes from compiling with C++ compiler:
> correct function signatures
> use explict casts from void*
> use NULL for zeroing pointers
> remove unneeded 'auto' from variables (also conflicts with use in C++)
> change function format from K&R to ansi
> other fixes:
> ws, style
This looks fantastic, but I'd beg to please separate "ws, style" changes into
separate commits ESPECIALLY when they're big commits with potentially subtle
logic changes. I know it's really hard to ignore them but there's a big cost
associated with intermixing them.
I actually read every line change of every commit during peer review (which all
core devs should be doing...). It's really really easy to just quickly scan a
diff when it's all stylistic. If there's logic changes intertwined even a
little bit, it makes for an excruciating time-consuming review. Any mistakes
become needles in a haystack (and there were a couple needles in r58851).
I suggest hitting up ws/style changes as a commit per dir either before working
changes in that dir or after you're done committing some code change(s). That
way all files in a dir get hit up consistently too.
Cheers!
Sean
------------------------------------------------------------------------------
Sponsored by Intel(R) XDK
Develop, test and display web and hybrid apps with a single code base.
Download it for free now!
http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk
_______________________________________________
BRL-CAD Developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/brlcad-devel
