Skip to content

Commit

Permalink
Repair PR #845: Scale vertical coords properly. (#848)
Browse files Browse the repository at this point in the history 
Thanks to Bas Couwenberg <sebastic@debian.org> for spotting this horrible bug!
  • Loading branch information
@busstoptaktik
busstoptaktik committed Mar 9, 2018
1 parent bae75e3 commit b027737
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/pj_transform.c
View file
Expand Up @@ -397,7 +397,9 @@ static int height_unit (PJ *P, PJ_DIRECTION dir, long n, int dist, double *z) {
fac = P->vfr_meter;

/* Nothing to do? */
if (P->vto_meter==0.0)
if (fac==1.0)
return 0;
if (0==z)

This comment has been minimized.

Sign in to view

Copy link
@sebastic

sebastic Mar 9, 2018

Contributor

Why 0==z instead of testing for NULL as in my change?

This comment has been minimized.

Sign in to view

Copy link
@busstoptaktik

busstoptaktik Mar 9, 2018

Author Member

The "0==ptr" idiom is used all over the code base (although NULL is used a lot too). Both are valid C89 - I handled your update by hand, and accidentally used the idiom I usually use. No offence intended, just the effect of muscle memory.

This comment has been minimized.

Sign in to view

Copy link
@sebastic

sebastic Mar 9, 2018

Contributor

Your idiom is quite unusual to me, and not something common in the proj sources (yet).

While it's unlikely that the NULL constant will be changed to a different value, your idiom will break when it does.

If I were in charge of code style I'd forbid it, but I'm not so I'll leave it at this.

This comment has been minimized.

Sign in to view

Copy link
@kbevers

kbevers Mar 9, 2018

Member

If I were in charge of code style I'd forbid it, but I'm not so I'll leave it at this.

Some day I'd like to introduce some more formal code style requirements but since the code base is largely still a mess I haven't found the courage to poke that beehive just yet. Suggestions are welcome though.

This comment has been minimized.

Sign in to view

Copy link
@busstoptaktik

busstoptaktik Mar 9, 2018

Author Member

Your idiom is quite unusual to me, and not something common in the proj sources (yet).

It is not my idiom, it is a common standard C idiom. It appears 433 times in the PROJ sources which is roughly 1,5 times as often as the NULL pointer comparison idiom.

The two idioms are semantically identical, according to the C89 standard, and as, e.g., explained by Andrew Keeton over at https://stackoverflow.com/questions/1296843/what-is-the-difference-between-null-0-and-0

If a pointer is being compared to the constant literal 0, then this is a check to see if the pointer is a null pointer. This 0 is then referred to as a null pointer constant. The C standard defines that 0 cast to the type void * is both a null pointer and a null pointer constant.

Additionally, to help readability, the macro NULL is provided in the header file stddef.h. Depending upon your compiler it might be possible to #undef NULL and redefine it to something wacky.

It is perfecly common C and forbidding it would render a large subset of existing code invalid. Additionally, as Andrew Keeton also mentions, the other convention is actually the dangerous one: Comparing a pointer to a literal 0 always works as intended, while comparing it to NULL depends on someone not redefining it to "something wacky".

A pointer compared to a literal 0 has special meaning in C. Like it or not. It's a common idiom, it's standard C, and someone redefining NULL will break the NULL cases - not the 0 cases, so your argument is wrong.

This comment has been minimized.

Sign in to view

Copy link
@sebastic

sebastic Mar 9, 2018

Contributor

The compiler treats NULL and 0 differently.

When someone changes NULL to e.g. -1 in stddef.h the code will keep doing the right thing when its called when z is NULL.

NULL is different from 0, the former means that there is no z value defined, the latter means that the height is zero (and may be changed to something else).

This comment has been minimized.

Sign in to view

Copy link
@busstoptaktik

busstoptaktik Mar 9, 2018

Author Member

Nonsense Bas. z is a pointer to double. Comparing it to 0 has the effect described in my previous note. The comparison is 0==z, not 0==*z. Please read carefully both the code and the previous note. Your debugging is correct, but your comments to my implementation are wrong. The last one even more wrong than the first.

Please re-read both the code and the link to Andrew's explanation on SO.
return 0;

for (i = 0; i < n; i++)
Expand Down

0 comments on commit b027737

Please sign in to comment.