June 15, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike Wey | On Friday, 13 June 2014 at 21:09:23 UTC, Mike Wey wrote:
> On 06/12/2014 09:30 PM, Rene Zwanenburg wrote:
>> I remember a function which does something like only only + canFind on
>> one go. It would look something like
>>
>> header.colorMapDepth.among(16, 32);
>>
>> but I can't find it right now.. Maybe it was only proposed but never added.
>
> http://dlang.org/library/std/algorithm/among.html
Will apply this as well, thanks :-)
|
June 16, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | I have refactored the code as recommended. I have also modified the not-yet-reviewed writers part to take advantage of the same approach (preallocated static-sized buffer) rather than allocate slices in loops. Hoping to hear something from you guys! Best, Mike |
June 16, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | On Monday, 16 June 2014 at 19:42:14 UTC, Mike wrote:
> I have refactored the code as recommended.
>
> I have also modified the not-yet-reviewed writers part to take advantage of the same approach (preallocated static-sized buffer) rather than allocate slices in loops.
>
> Hoping to hear something from you guys!
>
> Best,
> Mike
I've been pretty busy this weekend, so sorry for the late reply. I'm taking another look right now.
|
June 16, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | On Monday, 16 June 2014 at 19:42:14 UTC, Mike wrote: > I have refactored the code as recommended. > > I have also modified the not-yet-reviewed writers part to take advantage of the same approach (preallocated static-sized buffer) rather than allocate slices in loops. > > Hoping to hear something from you guys! > > Best, > Mike https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L80 Fully qualified names are only necessary when there's a name conflict. The std.algorithm can be removed. https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L8 It's a shame the result of among can't be returned directly but casts are terribly heavy handed. They're best reserved for when you're poking holes in the type system ;) For safe conversions use to!something: return to!bool(isColorMapped(header) ? header.colorMapDepth.among(16, 32) : header.pixelDepth.among(16, 32)); https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L29 This one depends on taste, but these helpers can be eliminated by changing the Header definition a little. It's the same union / anonymous struct trick from the previous post, only this time with bitfields: http://dpaste.dzfl.pl/13258b0ce0c4 https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L92 Hashtable lookup would indeed be nice here, but if you ever need a linear lookup it's included in std.algorithm: http://dlang.org/library/std/algorithm/countUntil.html const index = colorMap.countUntil(pixel); enforce(index >= 0, "Pixel color not found in color map: " ~ pixel.text); return index.to!ushort; Note the use of to!ushort to avoid the cast. to!ushort also performs overflow checking. https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L52 This is one of the many places where the allowed bitdepths are enumerated in an array / among(). Putting the allowed bitdepths in an enum has some advantages: use of '.max' for buffer sizes, EnumMembers in conjunction with among and only, to! checking for correctness... Some examples here: http://dpaste.dzfl.pl/30d2a593468f But it depends on your taste. Personally I prefer to use strong typing like this whenever possible. Helps to find bugs earlier, and makes maintenance easier when your software and data format are still in flux. https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L39 This should be checked both ways. The current method will not detect an error when the image type is not mapped but a color map is present. At least I assume that is not a valid TGA file? enforce(header.colorMapDepth.among(EnumMembers!ColorMapDepth)); enforce(header.isColorMapped == (header.colorMapType == ColorMapType.PRESENT)); enforce(header.isColorMapped == (header.colorMapDepth != ColorMapDepth.BPP0)); https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L95 You could change this to foreach(ref pixel; pixels) { pixel = ... } https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L165 Readability can be improved using the union + bitfields thingy. It's a bit longer but much easier on the eyes IMO: http://dpaste.dzfl.pl/77ecbafa1e0e https://github.com/emesx/TGA/blob/develop/source/tga/io/writers.d#L23 The with statement can be used here: with(image.header) { write(file, idLength); ... } I think this is everything I can come up with. The writeCompressed function can probably be simplified using std.algorithm but it's not straightforward. I'll need to ponder it for a while. |
June 17, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote:
> This one depends on taste, but these helpers can be eliminated by changing the Header definition a little. It's the same union / anonymous struct trick from the previous post, only this time with bitfields:
>
> http://dpaste.dzfl.pl/13258b0ce0c4
If you define nested structs/unions, it's better to make them static whenever possible, because non-static nested structs have an additional hidden field for the context.
Also, when you work with bit-(un)packing, you need to make sure it works on big- and little-endian CPUs.
|
June 17, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | Thanks, will work on fixes tonight. > The current method will not detect an error when the image type > is not mapped but a color map is present. > At least I assume that is not a valid TGA file? A non-mapped image may contain a color map ;-0 it's simply discarded. As per your ideas >return to!bool(isColorMapped(header) > ? header.colorMapDepth.among(16, 32) > : header.pixelDepth.among(16, 32)); this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea? |
June 18, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | On Tuesday, 17 June 2014 at 18:35:34 UTC, Mike wrote:
> Thanks, will work on fixes tonight.
>
>
>> The current method will not detect an error when the image type
>> is not mapped but a color map is present.
>> At least I assume that is not a valid TGA file?
>
> A non-mapped image may contain a color map ;-0 it's simply discarded.
>
>
> As per your ideas
>
>>return to!bool(isColorMapped(header)
>> ? header.colorMapDepth.among(16, 32)
>> : header.pixelDepth.among(16, 32));
>
> this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea?
Perhaps `return !!(...);` ?
|
June 18, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Marc Schütz | On Wednesday, 18 June 2014 at 09:41:01 UTC, Marc Schütz wrote:
> On Tuesday, 17 June 2014 at 18:35:34 UTC, Mike wrote:
>> Thanks, will work on fixes tonight.
>>
>>
>>> The current method will not detect an error when the image type
>>> is not mapped but a color map is present.
>>> At least I assume that is not a valid TGA file?
>>
>> A non-mapped image may contain a color map ;-0 it's simply discarded.
>>
>>
>> As per your ideas
>>
>>>return to!bool(isColorMapped(header)
>>> ? header.colorMapDepth.among(16, 32)
>>> : header.pixelDepth.among(16, 32));
>>
>> this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea?
>
> Perhaps `return !!(...);` ?
test
|
June 18, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | On Wednesday, 18 June 2014 at 13:42:32 UTC, Rene Zwanenburg wrote:
> On Wednesday, 18 June 2014 at 09:41:01 UTC, Marc Schütz wrote:
>> On Tuesday, 17 June 2014 at 18:35:34 UTC, Mike wrote:
>>> Thanks, will work on fixes tonight.
>>>
>>>
>>>> The current method will not detect an error when the image type
>>>> is not mapped but a color map is present.
>>>> At least I assume that is not a valid TGA file?
>>>
>>> A non-mapped image may contain a color map ;-0 it's simply discarded.
>>>
>>>
>>> As per your ideas
>>>
>>>>return to!bool(isColorMapped(header)
>>>> ? header.colorMapDepth.among(16, 32)
>>>> : header.pixelDepth.among(16, 32));
>>>
>>> this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea?
>>
>> Perhaps `return !!(...);` ?
>
> test
The web interface didn't work for me for about a day. It seems to be over now.
Yeah I didn't take nothrow into account. AFAIK using !!() to convert to bool in a context where it doesn't happen implicitly is indeed idiomatic. At least it's used in Phobos.
|
June 18, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote: > The writeCompressed function can probably be simplified using std.algorithm but it's not straightforward. I'll need to ponder it for a while. Here's a version using std.algorithm and std.range. I think it's easier to understand than the original: http://dpaste.dzfl.pl/88ba2592ac8d |
Copyright © 1999-2021 by the D Language Foundation