[pdf-devel] Re: pdf_i64_quick_assign

[Jeffrey Walton]

Hi Aleksander,

> I agree in that a name like "pdf_i64_set_i32" would be clearer.
> Not in the other things.
OK (but don't discount elegance).

>> something like pdf_i64_new_i32 might be more useful:
> Well, that is something that could also be added, and may be
> useful in cases like the one you say.
Its definitely serves the need of expediently creating a pdf_i64_t
from an pdf_i32_t. Since the function acts like pdf_i64_new, the
proposed has a similar signature and name. Also, since it cannot fail
(and does not take pointers), the function was reduced to a couple of
assignments. In the former, it will have to take pointers - which
means the parameters must be validated before use.

> I don't think that removing the function is the solution to the
> failed tests that we see.
Agreed. When they occur, failed tests are a separate issue. I've
encountered the massive breaking twice. The best I can tell, my lib
and check suite  are not on the same page (even after a 'make clean &&
make check'). I suspect its due to folding the multiple declarations
(between builtin i64 and BIGNUM) and implementations, and an error on
my part: defining a function while the macro (of the same name) is
still reachable.

> If any problem with the function, we should try to
> solve it and not just remove it...
The proposed change was more along 'interface cleanup'. It looks like
the pdf-types API has accumulated lint over its lifetime. I imagine
pdf_i64_div  was original, while pdf_i64_div_i32_dividend and
pdf_i64_div_i32_divisor are proliferation. Plus, there are no unit
tests (cf API Consistency Report) for the suspected add-ons, which
leads me to believe they were not part of the original specification.

In the end, I don't think the API is as clean and crisp as it was when
folks like you and Jose designed it. The library is very close to
going Production. Once in production, the API will be much more
difficult  to change (if it can be changed at all).

Jeff

OT: the macros need to go. I know beauty is in the eye of the
beholder, but they are ugly ducklings. They are not type safe (not
that I'm a big fan of C++ templates), and they can be a problem for
code generation. I spent about two hours trying to get gcc to produce
the code I wanted from the pdf_i64_subtract macro. gcc would produce
two signed compares ('jl' and 'jg') rather than the unsigned compares
('ja' and 'jb') that were required for overflow tests. I suspect it
was because I wanted to treat the same variable as both signed and
unsigned in the macro, and gcc wanted no part of it.

On Fri, Feb 12, 2010 at 5:02 AM, Aleksander Morgado
<address@hidden> wrote:
> Hi Jeff & list,
>
>>
>> Would you mind if this function went away? Here are a few reasons:
>>
>> * Non-elegant name
>> * Takes two pointers, void return
>> * There's nothing quick about its usage
>> * Would be better named <xxx>_i32, which is consistent with other 32
>> bit function names such as pdf_i64_add_i32 and  pdf_i64_mult_i32
>>
>
> I agree in that a name like "pdf_i64_set_i32" would be clearer. Not in
> the other things.
>
>> The function is used in two places: twice in pdf-time.c and twice in
>> pdf-fsys.c. Its usage looks similar to:
>>
>
> The function is not only to be used inside the library. It's in the API
> of the library, to provide a bignum implementation for systems which
> don't support it. Think of a printer which needs to process PDFs with
> the gnupdf library, for example.
>
>> pdf_status_t status;
>> pdf_i32_t small = -1;
>> pdf_i64_t big = ...;
>> pdf_i64_quick_assign(&big,small,&status);
>>
>> In its place, something like pdf_i64_new_i32 might be more useful:
>
> Well, that is something that could also be added, and may be useful in
> cases like the one you say.
>
>>
>> What do you think?
>>
>
> I don't think that removing the function is the solution to the failed
> tests that we see. If any problem with the function, we should try to
> solve it and not just remove it...
>
> BTW, please keep the mailing list always in CC.
>
> Cheers,
> -Aleksander
>
>