Tine subsurface updates

Jef Driesen jefdriesen at telenet.be
Fri May 4 06:55:27 PDT 2012


On 2012-05-03 17:58, Linus Torvalds wrote:
> On Thu, May 3, 2012 at 2:45 AM, Jef Driesen <jefdriesen at telenet.be> 
> wrote:
>> On 2012-05-02 19:35, Linus Torvalds wrote:
>>
>> I'm already working on some of those changes, along with the other 
>> items on
>> the roadmap [1,2] (namespace prefix, cleanups, etc). This will be a 
>> huge
>> break in backwards compatibility (e.g. the namespace prefix alone 
>> will
>> affect every single function).
>
> So don't worry about *that* break. I'll be dancing in the streets 
> when
> you break the compile because you've fixed the interfaces and they'll
> actually be something we can rely on.

Great, but as I explained in the previous mail, that break won't happen 
all at once. I tried doing it that way, and it's simply a pain in the 
ass to do both a major restructuring and regular bug fixing at the same 
time. That's why I want to have a stable release for use by 
applications, and the master tree for the work-in-progress stuff.

> It's the small, annoying, stupid and *unnecessary* breaking that
> drives me completely bonkers.
>
> The small stupid ones that expose some internal libdivecomputer stuff
> that shouldn't have been exposed in the first place, and are totally
> unnecessary, and are *not* good programming. Not for you, not for the
> user.
>
> And the latest Frog addition is a prime example.
>
> You used to have a function called hw_ostc_parser_create() that
> created the parser for the OSTC. Fine. The naming even explained what
> it did. But it is *insane* to change that function to take an extra
> argument that is just an integer that says "it's the Frog, not an
> OSTC".
>
> The reason it's insane is that it
>  (a) breaks existing users of the interface for no good reason
>  (b) isn't even the clean way of doing things
>  (c) now the naming makes no sense.
>
> and none of these problems would exist if you had just created a new
> "hw_frog_parser_create()" function, and left the old one alone. It
> would have been a *cleaner* interface. It would have resulted in more
> obvious code. It would actually document things better than just
> passing a random new integer to a now mis-named function.
>
> Sure,  you want to share 99% of the parser setup code, but that's
> irrelevant. That's an internal implementation issue, and could
> trivially and simply have been done by having an internal static
> function inside the library that nobody else would ever see:
>
>   enum hw_computers { HW_OSTC, HW_FROG };
>
>   static parser_status_t hw_parser_create (parser_t **out, int 
> model);
>
> (and note: none of the above would be exposed to anybody else: it's
> not in a header file, it's in one of your implementation C files) and
> then you'd just have done
>
>   parser_status_t hw_ostc_parser_create (parser_t **out)
>   {
>       return hw_parser_create(out, HW_OSTC);
>   }
>
>   parser_status_t hw_frog_parser_create (parser_t **out)
>   {
>       return hw_parser_create(out, HW_FROG);
>   }
>
> and now you'd have a *cleaner* interface to the outside world that
> wouldn't even have broken anything? Win-win.

I understand your point of view, and I even considered implementing it 
like that. But I didn't for two reasons:

The first one is that the hw_ostc_parser_create() function does not 
really create a parser for the ostc. It creates a parser for the ostc 
dataformat. Since the frog happens to use the same dataformat, it makes 
sense to reuse the same parser directly. There are several other 
examples where this happens. This mapping can be found in the source 
code, or the table on the website:

http://www.divesoftware.org/libdc/status.html

There was only one caveat. The frog stores the format version at a 
slightly different offset, and that's why I needed the extra info. I 
have reported this to Heinrichs Weikamp, but it was already too late to 
correct this in the firmware. Maybe I shouldn't have named the extra 
paramter "frog", but the more general "model". But that's just a name 
change.

Anyway the thing is that if you call hw_frog_parser_create(), you would 
expect to get back a frog parser (e.g. parser_get_type() should return 
PARSER_TYPE_HW_FROG, not PARSER_TYPE_HW_OSTC). I know you'll say that's 
an implementation detail that can be fixed, and you are right about 
that. But I didn't really bother doing so, for the next reason.

The second and more important reason is that for the development 
towards the next 0.2.0 version, I'll commit many changes that are far 
more invasive than this little frog breakage. So then what's the point 
of avoiding one little break when the majority of the next commits will 
break the api far worse. Especially when considering that after the 
whole restructuring is finished, you won't be needing this 
hw_ostc_parser_create() function anymore.

> It's the *ugly* and pointless breakage that I get so upset about. It
> makes things harder for everybody for no actual reason. And it is
> indicative of the same old "I don't think of libdivecomputer as a
> library, I think of it as a random collection of internal routines"
> problem.
>
> Interfaces are important. Even during development. They are doubly
> important when it's a library that other projects use.

I agree that the interfaces are important. I took your feedback very 
seriously when you complained to me about breaking backwards 
compatibility the first time (for the Mares Icon HD). I don't think I 
have ever broken backwards compatibility again since then.

Except for the latest frog change of course. But this time it was 
announced in advance, there is a stable release available, and last but 
not least there is a plan to move forward in the right direction. But if 
I'm not allowed to break stuff during the development phase, then the 
improvements we all want can't be added incrementally, and we are stuck 
with the current one. And that's exactly what I wanted to avoid.

Jef


More information about the subsurface mailing list