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