<div dir="ltr">Here is an incomplete take on <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">PJ_aeqd.c:</span><div><br></div><div><a href="https://gist.github.com/schwehr/2a1389d8d5b711ff9796530887247d2d">https://gist.github.com/schwehr/2a1389d8d5b711ff9796530887247d2d</a><br><div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 22, 2018 at 11:42 AM, Kristian Evers <span dir="ltr"><<a href="mailto:kristianevers@gmail.com" target="_blank">kristianevers@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class="gmail-"><br><blockquote type="cite"><div>On 22 Apr 2018, at 20:07, Kurt Schwehr <<a href="mailto:schwehr@gmail.com" target="_blank">schwehr@gmail.com</a>> wrote:</div><br class="gmail-m_-7981073164332894613Apple-interchange-newline"><div><div dir="ltr">Kristian,<br><div><br></div><div>Things to do when you are about to have a kid and want a distraction :)</div></div></div></blockquote><div><br></div></span>Congratulations!<br><br><blockquote type="cite"><div><div dir="ltr"><div>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></div><div>PJ_aeqd.c! There’s a bit more going on and It’s got 97.67% test coverage: </div><div><br></div><div><a href="https://coveralls.io/builds/16617275/source?filename=src/PJ_aeqd.c" target="_blank">https://coveralls.io/builds/<wbr>16617275/source?filename=src%<wbr>2FPJ_aeqd.c</a></div><div><div class="gmail-h5"><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div><div>ls -lSr PJ*.c | tail -15</div><div>-rw-r--r-- 1 schwehr eng 7654 Mar 20 10:49 PJ_omerc.c</div><div>-rw-r--r-- 1 schwehr eng 7659 Mar 21 07:51 PJ_cart.c</div><div>-rw-r--r-- 1 schwehr eng 7801 Mar 20 10:49 PJ_sch.c</div><div>-rw-r--r-- 1 schwehr eng 8444 Mar 20 10:49 PJ_stere.c</div><div>-rw-r--r-- 1 schwehr eng 8598 Mar 20 10:49 PJ_molodensky.c</div><div>-rw-r--r-- 1 schwehr eng 8685 Mar 29 12:41 PJ_axisswap.c</div><div>-rw-r--r-- 1 schwehr eng 9420 Mar 20 10:49 PJ_aeqd.c</div><div>-rw-r--r-- 1 schwehr eng 9907 Mar 20 10:49 PJ_deformation.c</div><div>-rw-r--r-- 1 schwehr eng 13068 Mar 6 12:50 PJ_qsc.c</div><div>-rw-r--r-- 1 schwehr eng 17186 Mar 20 10:49 PJ_unitconvert.c</div><div>-rw-r--r-- 1 schwehr eng 17405 Mar 29 12:41 PJ_pipeline.c</div><div>-rw-r--r-- 1 schwehr eng 17561 Mar 29 12:41 PJ_horner.c</div><div>-rw-r--r-- 1 schwehr eng 20639 Feb 6 08:46 PJ_helmert.c</div><div>-rw-r--r-- 1 schwehr eng 21130 Mar 20 10:49 PJ_healpix.c</div><div>-rw-r--r-- 1 schwehr eng 27407 Mar 20 10:49 PJ_isea.c</div></div><div><br></div><div>Would love to see what others come up with.</div><div><br></div><div class="gmail_extra"><div class="gmail_quote">On Sun, Apr 22, 2018 at 8:10 AM, Kristian Evers <span dir="ltr"><<a href="mailto:kreve@sdfe.dk" target="_blank">kreve@sdfe.dk</a>></span> wrote:<br><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>
<br>
You are brave to start a code style discussion :-) A few comments on your “rules”:<br></blockquote><div><br></div><div>Hah! Far from rules. I'm trying figure out what is even legal / works.</div><div> </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-m_-7981073164332894613gmail-"><br>
- Combine definition and declaration<br>
<br>
</span>It works in this example but I don’t think it can be enforced in more complicated code.<br>
Variable declarations have to be made at the start of a block and that is not always possible.<br>
<br></blockquote><div><br></div><div>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><br></div></div></div></div></div></blockquote><div><br></div></div></div><div>At least it is done throughout the code base but definitely not consistently. I think it is fine to do so.</div><span class="gmail-"><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </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>
<br>
I don’t have a strong opinion on this one, although in this example I mostly think the “const double”<br>
is cluttering up the code. What is gained by adding const here?<br></blockquote><div><br></div><div>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> </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-m_-7981073164332894613gmail-"><br>
- For double literals, have at least one digit before and after decimal point. e.g. .1 -> 0.1 and 3. -> 3.0<br>
<br>
</span>+1.<br>
<span class="gmail-m_-7981073164332894613gmail-"><br>
- Don't have assignments hidden inside expressions<br>
<br>
</span>Big +1!<br>
<span class="gmail-m_-7981073164332894613gmail-"><br>
- Convert #defines to typed const values<br>
<br>
</span>Would this be better as “static const …”?<br></blockquote><div><br></div><div>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> </div></div></div></div></div></blockquote><div><br></div></span><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><span class="gmail-"><br><blockquote type="cite"><div><div dir="ltr"><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-m_-7981073164332894613gmail-"><br>
- IWYU - Include what you use... here math.h<br>
<br>
</span>Another +1 here.<br>
<br>
<br>
Finally, I get an error when compiling your code:<br>
<br>
/Users/kevers/dev/proj4/src/PJ<wbr>_august.c:25:16: error: initializer for aggregate is not a compile-time<br>
constant [-Werror,-Wc99-extensions]<br>
<span class="gmail-m_-7981073164332894613gmail-"> M * x1 * (3.0 + x12 - 3.0 * y12),<br>
</span> ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~<wbr>~~<br>
1 error generated.<br>
<br></blockquote><div><br></div><div>Can you reproduce that failure in godbolt, or as least, what compiler/version/settings are you talking about?</div><div> </div></div></div></div></div></blockquote><div><br></div></span><div>This is how I set up my builds:</div><div><br></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/<wbr>Contents/Developer/Toolchains/<wbr>XcodeDefault.xctoolchain/usr/<wbr>bin/cc</div><div>-- Check for working C compiler: /Applications/Xcode.app/<wbr>Contents/Developer/Toolchains/<wbr>XcodeDefault.xctoolchain/usr/<wbr>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></div><div>Basically the same as what is done in the OSX Travis setup. </div></div></div><br><blockquote type="cite"><div><div><div class="gmail-h5"><div dir="ltr"><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>
PJ_COORD initialiser function.<br>
<br>
It would be interesting to see a more complicated function get the same treatment. <br>
<br>
/Kristian<br>
<div><div class="gmail-m_-7981073164332894613gmail-h5"><br>
<br>
> On 22 Apr 2018, at 15:08, Kurt Schwehr <<a href="mailto:schwehr@gmail.com" target="_blank">schwehr@gmail.com</a>> wrote:<br>
> <br>
> Totally missed that lp.lam assignment! Next iteration... also tried to add a const to the lp arg.<br>
> <br>
> <a href="https://godbolt.org/g/b4FUf7" rel="noreferrer" target="_blank">https://godbolt.org/g/b4FUf7</a><br>
> <br>
> 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>
> <br>
> On Sun, Apr 22, 2018 at 5:26 AM, Even Rouault <<a href="mailto:even.rouault@spatialys.com" target="_blank">even.rouault@spatialys.com</a>> wrote:<br>
> On samedi 21 avril 2018 15:46:22 CEST Kurt Schwehr wrote:<br>
> > Hi all,<br>
> ><br>
> > I've been thinking about what is possible with the Proj code base with an<br>
> > assumption that the code must be C89/C90 compatible. I played around for a<br>
> > few in godbolt with PJ_august.c (because it's small) and ended up with<br>
> > this. I tried to be aggressive as I could. I think my modified version is<br>
> > likely to be more static analyzer friendly.<br>
> <br>
> I hope they are intelligent enough to make sense of the original code ;-)<br>
> <br>
> > - Don't have assignments hidden inside expressions<br>
> <br>
> Definitely +1 on this !<br>
> <br>
> To avoid the modification of lp.lam in the cos(),<br>
> <br>
> > const double c = 1.0 + c1 * cos(lp.lam *= .5);<br>
> > const double x1 = sin(lp.lam) * c1 / c;<br>
> <br>
> this part could also be re-written, as<br>
> <br>
> const double half_lam = 0.5 * lp.lam;<br>
> const double c = 1.0 + c1 * cos(half_lam);<br>
> const double x1 = sin(half_lam) * c1 / c;<br>
> <br><br></div></div></blockquote></div><div class="gmail-m_-7981073164332894613gmail_signature"></div>
</div></div></div></div><span class="gmail-">
______________________________<wbr>_________________<br>Proj mailing list<br><a href="mailto:Proj@lists.maptools.org" target="_blank">Proj@lists.maptools.org</a><br><a href="http://lists.maptools.org/mailman/listinfo/proj" target="_blank">http://lists.maptools.org/<wbr>mailman/listinfo/proj</a></span></div></blockquote></div><br></div><br>______________________________<wbr>_________________<br>
Proj mailing list<br>
<a href="mailto:Proj@lists.maptools.org">Proj@lists.maptools.org</a><br>
<a href="http://lists.maptools.org/mailman/listinfo/proj" rel="noreferrer" target="_blank">http://lists.maptools.org/<wbr>mailman/listinfo/proj</a><br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">--<div><a href="http://schwehr.org" target="_blank">http://schwehr.org</a></div></div>
</div></div></div></div>