Re: [gpsd-dev] Weirdness in gpsrinex

[Fred Wright]

On Wed, 20 Mar 2019, Gary E. Miller wrote:
> On Wed, 20 Mar 2019 12:35:45 -0700 (PDT)
> Fred Wright <address@hidden> wrote:
>
> > I was looking into some warnings in gpsrinex, but ran across
> > something where it's really unclear what the intent was.  Both
> > warnings relate to the obs_cnt array defined in lines 138-143.
>
> Where do you see these warnings? gcc and 7.4.0m 8.2.0 and 8.3.0
> compile clean for me.

I've seen them on several platforms, including the Mac (where I work 
primarily).  Adnittedly not on *all* platforms.

They all represent legitmate problems.

> > One warning is just for the missing initializer braces.  That one's
> > easy - it should have two levels of brace rather than one (one for
> > the array and one for the struct).
>
> Feel free to push a fix.

I will, but along with the other fixes for gpsrinex.

> > The weird one concerns the comparison in line 210 against the
> > obs_cnts element.  It looks like the obs_codes enum should be the
> > type of the *index* to the obs_cnts array, but it shouldn't be the
> > type of its *value*, which most likely should be unsigned int.
>
> The gpsrinex code works, but that is an error check, so likely untested.

I thought that it wasn't just an error check, but a cap on the value. 
Admittedly, getting the count to pass 99999 would probably be fairly 
unusual. :-)

> It does look like obs_cnt_t obs_cnts should be a count, not an enum.
> The enum is the index.  But since C uses ints for enums it ends up
> working anyway.

The reason for the warning is that it's comparing against 99999, but the 
enum range is known to be much less than that, so the compiler is 
complaining about what should be an impossible condition.

> > Meanwhile, there's a separate unindexed count element which doesn't
> > seem to be used anywhere.
>
> I assume you mean obs_cnt_t count?  I also don't see it used.
>
> > The obvious fix is to fix the type of obs_cnts and remove the unused
> > count element, but I'm not sure if there's something else going on
> > that I'm missing.
>
> I think you are correct.

OK, I'll do that.

> It would be nice if gpsrinex had a regression test...  If you patch it,
> then I can easily test it.

Yeah, fitting it into the existing test framework wouldn't be *too* hard, 
but not trivial.

Fred Wright