www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Working on a library: request for code review

reply "Mike" <mike mike.com> writes:
Hello.

I am new to D and I must admit I really like the language. In my
opinion it takes the best from C++ and, say, Python and combines
it really elegantly. Great work!

I am currently working on my first library in D - related to
TARGA image format.

Here's the link to the repo: http://bit.ly/1mIuGhv

It's a work-in-progress case: at the moment the library does what
I need for my other projects, but there is a couple of things
that I want to add/fix soon.

Perhaps someone could have a look at the code and point out some
obvious traps that I fell into etc.

Any feedback would be great!

Best regards,
Mike
Jun 11 2014
next sibling parent reply "cal" <callumenator gmail.com> writes:
On Wednesday, 11 June 2014 at 18:29:27 UTC, Mike wrote:

 Hello.
 Here's the link to the repo: http://bit.ly/1mIuGhv


Hi, sorry didn't read through your code yet, but while ago I 
wrote some encoders/decoders for jpeg and png 
(https://github.com/callumenator/imaged, haven't compiled it in a 
while). Might it be worth stitching things together into a proper 
image processing package?

Cheers,
cal
Jun 11 2014
parent reply "Mike" <mike mike.com> writes:
On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:

 Might it be worth stitching things together into a proper image 
 processing package?


Well I started working on TGA because I was disappointed that no 
image abstraction is present in Phobos. Go has some imaging APIs 
and I think D would benefit from having one too (out of the box).

Would I work on std.image? Sure.

Best,
Mike
Jun 12 2014
parent reply "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
On Thursday, 12 June 2014 at 15:46:12 UTC, Mike wrote:

 On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:

 Might it be worth stitching things together into a proper 
 image processing package?


 Well I started working on TGA because I was disappointed that 
 no image abstraction is present in Phobos. Go has some imaging 
 APIs and I think D would benefit from having one too (out of 
 the box).

 Would I work on std.image? Sure.

 Best,
 Mike


I'm looking over your code ATM but I'd like to reply to this 
first.

IMO it's not a good idea to create something like std.image. The 
std lib should ideally never have breaking changes, so it's easy 
to get stuck with a sub optimal API or become increasingly hard 
to maintain.

We have Dub. Better keep the std lib lean and maintainable. If 
you're looking to create an awesome idiomatic D image library 
you're probably better of building it on top of derelict-devil or 
derelict-freeimage, then publish it on code.dlang.org

The discoverability of good code.dlang.org projects is still 
limited. Some kind of rating or like system would be useful, but 
that's another discussion.
Jun 12 2014
parent reply Xavier Bigand <flamaros.xavier gmail.com> writes:
Le 12/06/2014 20:09, Rene Zwanenburg a écrit :

 On Thursday, 12 June 2014 at 15:46:12 UTC, Mike wrote:

 On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:

 Might it be worth stitching things together into a proper image
 processing package?


 Well I started working on TGA because I was disappointed that no image
 abstraction is present in Phobos. Go has some imaging APIs and I think
 D would benefit from having one too (out of the box).

 Would I work on std.image? Sure.

 Best,
 Mike


 I'm looking over your code ATM but I'd like to reply to this first.

 IMO it's not a good idea to create something like std.image. The std lib
 should ideally never have breaking changes, so it's easy to get stuck
 with a sub optimal API or become increasingly hard to maintain.

 We have Dub. Better keep the std lib lean and maintainable. If you're
 looking to create an awesome idiomatic D image library you're probably
 better of building it on top of derelict-devil or derelict-freeimage,
 then publish it on code.dlang.org

 The discoverability of good code.dlang.org projects is still limited.
 Some kind of rating or like system would be useful, but that's another
 discussion.


I think it can be a great advantage to have some things like image 
management in phobos, cause often dub projects are big and users don't 
want necessary a complete multimedia library but just small pieces that 
are standard.
For example a GUI library, will allow image manipulations, but not only, 
and extracting only the image modules can be hard. That the case of our 
DQuick project.

For a project like DQuick, I would be happy to find things like images, 
geometric algebra,environment analysis,... in phobos. This will allow 
use to be focused on other things making an UI framework (Windows, 
events, rendering, resource management,...).

Having such minimalistic APIs would be a great benefit IMO, maybe in 
this case some extending libraries would appear in dub.
Jun 12 2014
parent reply Xavier Bigand <flamaros.xavier gmail.com> writes:
Le 12/06/2014 20:35, Xavier Bigand a écrit :

 Le 12/06/2014 20:09, Rene Zwanenburg a écrit :

 On Thursday, 12 June 2014 at 15:46:12 UTC, Mike wrote:

 On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:

 Might it be worth stitching things together into a proper image
 processing package?


 Well I started working on TGA because I was disappointed that no image
 abstraction is present in Phobos. Go has some imaging APIs and I think
 D would benefit from having one too (out of the box).

 Would I work on std.image? Sure.

 Best,
 Mike


 I'm looking over your code ATM but I'd like to reply to this first.

 IMO it's not a good idea to create something like std.image. The std lib
 should ideally never have breaking changes, so it's easy to get stuck
 with a sub optimal API or become increasingly hard to maintain.

 We have Dub. Better keep the std lib lean and maintainable. If you're
 looking to create an awesome idiomatic D image library you're probably
 better of building it on top of derelict-devil or derelict-freeimage,
 then publish it on code.dlang.org

 The discoverability of good code.dlang.org projects is still limited.
 Some kind of rating or like system would be useful, but that's another
 discussion.


 I think it can be a great advantage to have some things like image
 management in phobos, cause often dub projects are big and users don't
 want necessary a complete multimedia library but just small pieces that
 are standard.
 For example a GUI library, will allow image manipulations, but not only,
 and extracting only the image modules can be hard. That the case of our
 DQuick project.

 For a project like DQuick, I would be happy to find things like images,
 geometric algebra,environment analysis,... in phobos. This will allow
 use to be focused on other things making an UI framework (Windows,
 events, rendering, resource management,...).

 Having such minimalistic APIs would be a great benefit IMO, maybe in
 this case some extending libraries would appear in dub.


I am thinking an other benefit is what you see as a default, the 
assurance of D standard conformances (portability, safety, quality, 
support,...).
Jun 12 2014
parent "Mike" <mike mike.com> writes:
I can shed some light on my, beginners, point of view and the 
rationale behind this tiny library.

I want to create a bar/matrix-code generator (QR, DataMatrix 
etc.). I really like graphics-related subjects and I thought this 
would be a good start in D.

Coming from Java island and having experience in Go I expected to 
find some basic imaging functionalities in the standard library: 
not necessary support for all image formats, but at least some 
bitmap i/o and data model (Pixel, Image, Filter ...).

I found none of that :-(

IMHO (one of the) pain(s) of C++ is that the stdlib is far behind 
what a modern developer would consider "elementary".

But I expected something from D, because Phobos is already more 
than C++'s stdlib is: Phobos has e.g. digests. IMO digests are 
not (as) necessary for a standard library as, say, a qsort is. So 
if there are digests, why not imaging?

Either way, D is really nice and if I can help, I will :-) So far 
- the TGA lib.

Best,
Mike
Jun 12 2014
prev sibling parent reply "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
On Wednesday, 11 June 2014 at 18:29:27 UTC, Mike wrote:

 Here's the link to the repo: http://bit.ly/1mIuGhv


I usually don't trust shortened URL's. Can you please post full 
URL's when not constrained by a character limit?


 Any feedback would be great!


First of all, I like your coding style. It's nice and readable :)

https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d

All those functions can be marked pure. Pure is D is a bit 
different than in other languages. I can recommend the following 
article as a good explanation:

http://klickverbot.at/blog/2012/05/purity-in-d/

Also, depending on your preference, you may want to put those 
functions in a

pure nothrow
{

}

block, or put

pure nothrow:

at the top of the source file. I'm not sure if all those 
functions can be nothrow though.


https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L8

These array literals will be allocated every time the function is 
called. 'only' can be used instead:

only(16, 32).canFind // etc..

http://dlang.org/library/std/range/only.html

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.


https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L101

This has a pretty bad time complexity. In the worst case this is 
O(n^2), I think. You could use a hashmap to build the table, then 
convert to an array. Or even only use hashmaps as color tables 
internally to improve performance across the board.


https://github.com/emesx/TGA/blob/develop/source/tga/model/types.d#L11

Depending on your taste, a union + anonymous struct could be used:

struct Pixel
{
	union
	{
		ubyte[4] bytes;
		
		struct
		{
			ubyte b, g, r, a;
		}
	}
}


https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L47

to!string is so common it has a special alias: text. It's placed 
in std.conv so you still need to import that.

"Invalid color map pixel depth: " ~ header.colorMapDepth.text


https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L24

Yeah, we need something to read an entire struct from a file and 
convert it to the correct endianness..


https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L24
https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L91

Unnecessary GC allocation in a hot loop == :(
Make a buffer outside the loop and call the normal rawRead:

auto buffer = new ubyte[colorMapByteDepth];
foreach(i; 0 .. (header.colorMapLength - header.colorMapOffset))
{
	file.rawRead(buffer);
	colorMap[i] = pixelReader(buffer);
}

Even better, use a static array with the max byte depth, then 
slice to the correct length:

ubyte[MaxByteDepth] buffer;
foreach(i; 0 .. (header.colorMapLength - header.colorMapOffset))
{
	colorMap[i] = file.rawRead(buffer[0 .. 
colorMapByteDepth]).pixelReader;
}

Do the same thing here:
https://github.com/emesx/TGA/blob/develop/source/tga/io/utils.d#L5

T read(T)(File file) if(isNumeric!T){
     ubyte[T.sizeof] bytes;
     file.rawRead(s[]);
     return littleEndianToNative!T(bytes);
}


https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L144

Depending on your taste. The first is built in, the other needs 
std.algorithm.

pixel.bytes[] = chunk[];
chunk.copy(pixel.bytes);


https://github.com/emesx/TGA/blob/develop/source/tga/io/utils.d#L40

There's a version in std.algorithm:
http://dlang.org/phobos/std_algorithm.html#min


I need to go. Please don't mind any typo's and untested code ;). 
Didn't take a look at writers yet, readers and util need some 
more scrutiny, but the main theme is: eliminate unnecessary 
temporary GC allocations.
Jun 12 2014
next sibling parent "Mike" <mike mike.com> writes:
On Thursday, 12 June 2014 at 19:30:06 UTC, Rene Zwanenburg wrote:

 I need to go. Please don't mind any typo's and untested code 
 ;). Didn't take a look at writers yet, readers and util need 
 some more scrutiny, but the main theme is: eliminate 
 unnecessary temporary GC allocations.


Thank you for feedback, hoping for more! I will apply the 
suggestions tonight.
Jun 13 2014
prev sibling parent reply Mike Wey <mike-wey example.com> writes:
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

-- 
Mike Wey
Jun 13 2014
parent reply "Mike" <mike mike.com> writes:
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 :-)
Jun 15 2014
parent reply "Mike" <mike mike.com> writes:
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
Jun 16 2014
next sibling parent "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
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.
Jun 16 2014
prev sibling parent reply "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
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.
Jun 16 2014
next sibling parent reply "Marc =?UTF-8?B?U2Now7x0eiI=?= <schuetzm gmx.net> writes:
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.
Jun 17 2014
parent reply "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
On Tuesday, 17 June 2014 at 13:07:33 UTC, Marc Schütz wrote:

 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.


Good call. In this case it doesn't add the hidden field, but for 
the sake of consistency I agree it's better to add static.


 Also, when you work with bit-(un)packing, you need to make sure 
 it works on big- and little-endian CPUs.


Yeah. He's using a custom read function that takes endianness 
into account, so I didn't mention it.
Jun 18 2014
parent reply "Marc =?UTF-8?B?U2Now7x0eiI=?= <schuetzm gmx.net> writes:
On Wednesday, 18 June 2014 at 14:05:12 UTC, Rene Zwanenburg wrote:

 On Tuesday, 17 June 2014 at 13:07:33 UTC, Marc Schütz wrote:

 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.


 Good call. In this case it doesn't add the hidden field, but 
 for the sake of consistency I agree it's better to add static.


Interesting, I thought it's there for all nested structs. But I 
see it's only added if the struct has a method, but then it's 
always there, no matter whether the methods actually accesses the 
outer context. Probably for consistency reasons...
Jun 19 2014
parent reply "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
On Thursday, 19 June 2014 at 12:06:58 UTC, Marc Schütz wrote:

 On Wednesday, 18 June 2014 at 14:05:12 UTC, Rene Zwanenburg 
 wrote:

 On Tuesday, 17 June 2014 at 13:07:33 UTC, Marc Schütz wrote:

 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.


 Good call. In this case it doesn't add the hidden field, but 
 for the sake of consistency I agree it's better to add static.


 Interesting, I thought it's there for all nested structs. But I 
 see it's only added if the struct has a method, but then it's 
 always there, no matter whether the methods actually accesses 
 the outer context. Probably for consistency reasons...


The absence of the hidden field without methods is probably to 
maintain C compitability; code that is both valid C and D must 
have the same semantics. I think that includes struct layout?
Jun 19 2014
next sibling parent "Mike" <mike mike.com> writes:
Once again thanks for the feedback. The RLE rewrite using std is 
impressive! I will use it today or tomorrow, great stuff.
Jun 19 2014
prev sibling parent reply "Mike" <mike mike.com> writes:
Do you think it's ready for a v0.1 release? It be willing to add 
it to dub if it passes general D coding stardards.

Once again, thanks for feedback and vast improvements of the code.

Best,
Mike
Jun 20 2014
parent "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
On Friday, 20 June 2014 at 11:15:20 UTC, Mike wrote:

 Do you think it's ready for a v0.1 release? It be willing to 
 add it to dub if it passes general D coding stardards.


Sure, go for it :). Dub package versioning should be done using 
SemVer so there are some restrictions on how to number a release.

http://semver.org/
Jun 21 2014
prev sibling next sibling parent reply "Mike" <mike mike.com> writes:
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?
Jun 17 2014
parent reply "Marc =?UTF-8?B?U2Now7x0eiI=?= <schuetzm gmx.net> writes:
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 !!(...);` ?
Jun 18 2014
parent reply "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
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
Jun 18 2014
parent "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
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.
Jun 18 2014
prev sibling parent "Rene Zwanenburg" <renezwanenburg gmail.com> writes:
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
Jun 18 2014