<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 22 Apr 2018, at 20:07, Kurt Schwehr <<a href="mailto:schwehr@gmail.com" class="">schwehr@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Kristian,<br class=""><div class=""><br class=""></div><div class="">Things to do when you are about to have a kid and want a distraction :)</div></div></div></blockquote><div><br class=""></div>Congratulations!<br class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">I wonder what would be a good/interesting larger case to play with? Preferably one with good test coverage…</div></div></div></blockquote><div><br class=""></div><div>PJ_aeqd.c! There’s a bit more going on and It’s got 97.67% test coverage: </div><div><br class=""></div><div><a href="https://coveralls.io/builds/16617275/source?filename=src/PJ_aeqd.c" class="">https://coveralls.io/builds/16617275/source?filename=src%2FPJ_aeqd.c</a></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class=""><div class="">ls -lSr PJ*.c | tail -15</div><div class="">-rw-r--r-- 1 schwehr eng 7654 Mar 20 10:49 PJ_omerc.c</div><div class="">-rw-r--r-- 1 schwehr eng 7659 Mar 21 07:51 PJ_cart.c</div><div class="">-rw-r--r-- 1 schwehr eng 7801 Mar 20 10:49 PJ_sch.c</div><div class="">-rw-r--r-- 1 schwehr eng 8444 Mar 20 10:49 PJ_stere.c</div><div class="">-rw-r--r-- 1 schwehr eng 8598 Mar 20 10:49 PJ_molodensky.c</div><div class="">-rw-r--r-- 1 schwehr eng 8685 Mar 29 12:41 PJ_axisswap.c</div><div class="">-rw-r--r-- 1 schwehr eng 9420 Mar 20 10:49 PJ_aeqd.c</div><div class="">-rw-r--r-- 1 schwehr eng 9907 Mar 20 10:49 PJ_deformation.c</div><div class="">-rw-r--r-- 1 schwehr eng 13068 Mar 6 12:50 PJ_qsc.c</div><div class="">-rw-r--r-- 1 schwehr eng 17186 Mar 20 10:49 PJ_unitconvert.c</div><div class="">-rw-r--r-- 1 schwehr eng 17405 Mar 29 12:41 PJ_pipeline.c</div><div class="">-rw-r--r-- 1 schwehr eng 17561 Mar 29 12:41 PJ_horner.c</div><div class="">-rw-r--r-- 1 schwehr eng 20639 Feb 6 08:46 PJ_helmert.c</div><div class="">-rw-r--r-- 1 schwehr eng 21130 Mar 20 10:49 PJ_healpix.c</div><div class="">-rw-r--r-- 1 schwehr eng 27407 Mar 20 10:49 PJ_isea.c</div></div><div class=""><br class=""></div><div class="">Would love to see what others come up with.</div><div class=""><br class=""></div><div class="gmail_extra"><div class="gmail_quote">On Sun, Apr 22, 2018 at 8:10 AM, Kristian Evers <span dir="ltr" class=""><<a href="mailto:kreve@sdfe.dk" target="_blank" class="">kreve@sdfe.dk</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kurt,<br class="">
<br class="">
You are brave to start a code style discussion :-) A few comments on your “rules”:<br class=""></blockquote><div class=""><br class=""></div><div class="">Hah! Far from rules. I'm trying figure out what is even legal / works.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br class="">
- Combine definition and declaration<br class="">
<br class="">
</span>It works in this example but I don’t think it can be enforced in more complicated code.<br class="">
Variable declarations have to be made at the start of a block and that is not always possible.<br class="">
<br class=""></blockquote><div class=""><br class=""></div><div class="">Agreed. Not always possible or always a good thing. And a style question is do you allow variables to be defined in sub scopes? In C++, the answer is usually to keep things as tight/narrow as possible. In Proj, Idonno.</div><div class=""><br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>At least it is done throughout the code base but definitely not consistently. I think it is fine to do so.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- Add const<br class="">
<br class="">
I don’t have a strong opinion on this one, although in this example I mostly think the “const double”<br class="">
is cluttering up the code. What is gained by adding const here?<br class=""></blockquote><div class=""><br class=""></div><div class="">Here there isn't much value, but if you come into this function cold, you know immediately that there aren't sneaky things going on.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br class="">
- For double literals, have at least one digit before and after decimal point. e.g. .1 -> 0.1 and 3. -> 3.0<br class="">
<br class="">
</span>+1.<br class="">
<span class="gmail-"><br class="">
- Don't have assignments hidden inside expressions<br class="">
<br class="">
</span>Big +1!<br class="">
<span class="gmail-"><br class="">
- Convert #defines to typed const values<br class="">
<br class="">
</span>Would this be better as “static const …”?<br class=""></blockquote><div class=""><br class=""></div><div class="">In C++, I would just use constexpr. In C, I'm guessing that for -O2 and higher, it won't change anything. Leaving off the static might let the compiler be more aggressive. But this is why using godbolt is pretty awesome. You can try it and see if there is any difference and, if so, which way is more efficient. A micro benchmark might be useful too.</div><div class=""> </div></div></div></div></div></blockquote><div><br class=""></div><div>I can’t see a difference between using static and not. The assembler code is mostly gibberish to me though so I might have missed something.</div><div>Using the static keyword is probably not necessary then.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br class="">
- IWYU - Include what you use... here math.h<br class="">
<br class="">
</span>Another +1 here.<br class="">
<br class="">
<br class="">
Finally, I get an error when compiling your code:<br class="">
<br class="">
/Users/kevers/dev/proj4/src/<wbr class="">PJ_august.c:25:16: error: initializer for aggregate is not a compile-time<br class="">
constant [-Werror,-Wc99-extensions]<br class="">
<span class="gmail-"> M * x1 * (3.0 + x12 - 3.0 * y12),<br class="">
</span> ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~<wbr class="">~~<br class="">
1 error generated.<br class="">
<br class=""></blockquote><div class=""><br class=""></div><div class="">Can you reproduce that failure in godbolt, or as least, what compiler/version/settings are you talking about?</div><div class=""> </div></div></div></div></div></blockquote><div><br class=""></div><div>This is how I set up my builds:</div><div><br class=""></div><div><div>$ CFLAGS="-std=c89 -g -Wall -Wextra -Werror -Wunused-parameter -Wmissing-prototypes -Wmissing-declarations -Wformat -Werror=format-security -Wshadow -Wfloat-conversion -O2" cmake ../../proj4/</div><div>-- The C compiler identification is AppleClang 9.1.0.9020039</div><div><div>-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc</div><div>-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works</div><div>-- Detecting C compiler ABI info</div><div>-- Detecting C compiler ABI info - done</div><div>-- Detecting C compile features</div><div>-- Detecting C compile features - done</div><div>…</div><div><br class=""></div><div>Basically the same as what is done in the OSX Travis setup. </div></div></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
So for that you would have to assign xy.x and xy.y individually. Or possible make use of the proj_coord()<br class="">
PJ_COORD initialiser function.<br class="">
<br class="">
It would be interesting to see a more complicated function get the same treatment. <br class="">
<br class="">
/Kristian<br class="">
<div class=""><div class="gmail-h5"><br class="">
<br class="">
> On 22 Apr 2018, at 15:08, Kurt Schwehr <<a href="mailto:schwehr@gmail.com" class="">schwehr@gmail.com</a>> wrote:<br class="">
> <br class="">
> Totally missed that lp.lam assignment! Next iteration... also tried to add a const to the lp arg.<br class="">
> <br class="">
> <a href="https://godbolt.org/g/b4FUf7" rel="noreferrer" target="_blank" class="">https://godbolt.org/g/b4FUf7</a><br class="">
> <br class="">
> And as for the static analize, try to imagine that this is being done on a much more complicated function, because, yes, if a static analyzer didn't get this function, it's pretty much useless.<br class="">
> <br class="">
> On Sun, Apr 22, 2018 at 5:26 AM, Even Rouault <<a href="mailto:even.rouault@spatialys.com" class="">even.rouault@spatialys.com</a>> wrote:<br class="">
> On samedi 21 avril 2018 15:46:22 CEST Kurt Schwehr wrote:<br class="">
> > Hi all,<br class="">
> ><br class="">
> > I've been thinking about what is possible with the Proj code base with an<br class="">
> > assumption that the code must be C89/C90 compatible. I played around for a<br class="">
> > few in godbolt with PJ_august.c (because it's small) and ended up with<br class="">
> > this. I tried to be aggressive as I could. I think my modified version is<br class="">
> > likely to be more static analyzer friendly.<br class="">
> <br class="">
> I hope they are intelligent enough to make sense of the original code ;-)<br class="">
> <br class="">
> > - Don't have assignments hidden inside expressions<br class="">
> <br class="">
> Definitely +1 on this !<br class="">
> <br class="">
> To avoid the modification of lp.lam in the cos(),<br class="">
> <br class="">
> > const double c = 1.0 + c1 * cos(lp.lam *= .5);<br class="">
> > const double x1 = sin(lp.lam) * c1 / c;<br class="">
> <br class="">
> this part could also be re-written, as<br class="">
> <br class="">
> const double half_lam = 0.5 * lp.lam;<br class="">
> const double c = 1.0 + c1 * cos(half_lam);<br class="">
> const double x1 = sin(half_lam) * c1 / c;<br class="">
> <br class=""><br class=""></div></div></blockquote></div><div class="gmail_signature"></div>
</div></div>
_______________________________________________<br class="">Proj mailing list<br class=""><a href="mailto:Proj@lists.maptools.org" class="">Proj@lists.maptools.org</a><br class="">http://lists.maptools.org/mailman/listinfo/proj</div></blockquote></div><br class=""></body></html>