revid/config: correct types in config

[Saxon Milton]

We would like to use uints where appropriate i.e. for config parameters that will never be negative. There are some parameters that are not doing this, we should fix.

[Trek Hopton]

I’m not sure that we should be using uint whenever we want non-negative values. I think the general rule is to use ints unless you specifically need a uint.

I remember seeing this here: https://tour.golang.org/basics/11

And I just found a discussion on it: https://www.reddit.com/r/golang/comments/8plmo0/use_int_as_the_type_if_a_number_will_never_be/

Maybe @kortschak can weigh in here?

IMO it just makes us have to do more conversions when doing calculations, and using interfaces and packages that don’t use uints for non-negatives, (which seems like most).

[kortschak]

Using an unsigned integer for a value that cannot be negative gives a potentially false sense of security. As is always the case, the context makes a difference.

[Saxon Milton]

We have discussed this briefly before. These are fields in the exported Config struct that revid uses that should not be negative e.g. bitrates, widths, heights etc. Does it make sense to make these uints ?

[kortschak]

Yes, I recall the discussion vaguely. It depends on whether the values are mutated other than by setting and what other bounds checking is needed.

[Trek Hopton]

If you’d like some context, from what I understand (correct me if I’m wrong Saxon), we give VidGrind config parameters with a map of their data types so that they can be checked eg. string for OutputPath, float for RecordPeriod. Revid’s integer config params that can’t be negative have been given uint in the map so that VidGrind knows to make sure they’re not <= 0, but their datatype in revid isn’t necessarily uint, a lot of them have been int.

We could:

A. Change all the int revid config params that aren’t negative to uint.

B. Change the DataType map to only specify int but then give a range which the integer should be so vidgrind can still check for sensible values.

If we choose ‘A', should we then start using uints in all of our code where ints shouldn’t be negative for consistency, or still use ints everywhere but convert them to uints for the revid config params?

[Saxon Milton]

The values are set and then only mutated if fields are invalid and defaults are used.

In the relevant PR I have also used the same reasoning for the Config struct in the pcm pkg, so as to reduce conversions from the revid config fields to the pcm config fields. This does result in a little more conversion to ints when the config values are used in math; I think this is what Trek doesn’t like.

Also, I’ve made some changes to the device/geovision/config pkg i.e. option functions take uints instead of ints for values that should not be negative. Similarly the values are not mutated, just used to configure the camera.

[Saxon Milton]

Resolved in PR #401