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 :-)

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

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.

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.

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.

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?

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 !!(...);` ?

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

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.

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