On Tue, May 3, 2016 at 3:36 PM, Mark Thompson <[email protected]> wrote: > On 03/05/16 16:50, Vittorio Giovara wrote: >> On Tue, May 3, 2016 at 7:25 AM, Anton Khirnov <[email protected]> wrote: >>> Quoting Kieran Kunhya (2016-05-03 11:33:42) >>>> On Tue, 3 May 2016 at 07:43 Luca Barbato <[email protected]> wrote: >>>> >>>>> On 03/05/16 15:34, Kieran Kunhya wrote: >>>>>> I disagree, the old names are relatively clear. Whilst I think the speed >>>>>> improvements in this patch are great, the function names like >>>>> bitstream_read_32 >>>>>> are really confusing. IMO adding a number suffix should be the exception >>>>>> rather than the norm (i.e when reading large numbers of bits). >>>>> >>>>> The past code shown that not having the number of bits would make people >>>>> assume such functions work for the wrong range. >>>>> >>>>> The new functions support a larger range BUT I had bitten once too many >>>>> to consider using _long for the 63 bits variant. >>>>> >>>>> Yes but reading > 32 bits isn't very common so it should be treated as the >>>> special case. >>>> All these _32s make things very very unreadable. I want the unusual cases >>>> to have special suffixes. >>> >>> I'm not buying those "common" vs "uncommon" arguments. Experience shows >>> that people get it wrong all the time with the current code, so the new >>> API should make it very explicit what limitation does each variant have. >> >> IMO Anton is right wrt common/uncommon, but Kieran is right for the >> trailing number being confusing. >> I would propose bitstream_uintread and bitstream_longread which would >> make it perfectly clear the maximum length of the read value. > > > I think the most common case is "I want to read some bits, often a constant > number but maybe not, and I'd like it to be fast but if I sacrifice some > marginal speed for readability then fine". > > So, how about: > > #if GNU_SOMETHING > > #define bitstream_read(ctx, len) \ > (__builtin_constant_p(len) ? ((len) < 0 ? barf() : \ > (len) == 0 ? 0 : \ > (len) == 1 ? bitstream_read_bit(ctx) : \ > (len) < 32 ? bitstream_read_32((ctx), (len)) > : \ > (len) < 64 ? bitstream_read_63((ctx), (len)) > : \ > barf()) : \ > bitstream_read_63((ctx), (len))) > > #else > > #define bitstream_read(ctx, len) bitstream_read_63((ctx), (len)) > > #endif > > ? > > Cases where performance is very important can choose the correct inner > function as appropriate. Everyone else will usually get the right one > anyway, and not have to be careful about the internals and exactly how wide > things are. > > (Also maybe something microsofty as another variant, I don't know how that > works.)
I like this idea very very much. -- Vittorio _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
