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.
