I've posted my version of a tga to df3 converter in
povray.binaries.utilities. 

Source code is in C++. Constructive criticism on the source is welcome. I
probably over did some things and maybe blindly missed others. It works for
me, that's about as much as I can say.

No binaries are included in the archive, so a recent C++ compiler will be
needed. All testing was with GNU C++ compiler in linux and windows.

See the README for way more than you want to know. 

Thanks, I hope it's ok!
Ross

Post a reply to this message

Ross <rli### [at] everestkcnet> wrote:
> Source code is in C++. Constructive criticism on the source is welcome. I
> probably over did some things and maybe blindly missed others.

  1191 lines of code in 16 files for converting TGA images to DF3
certainly feels a bit like overdoing things... :)
  But it isn't that bad, really.

  I do not understand why you need to allocate an instance of arg::CL_Args
dynamically at the beginning of main().
  Is there a reason why this won't do?

arg::CL_Args args(argc, argv);

  Unnecessarily allocating it dynamically is only a source of problems and
has no benefits.
  And in fact, you *have* one such problem in your code: There are several
exit points in the program where the instance is not freed. If you had
used a local instance there wouldn't be such problem. You also (quite
inconsistently) free it in one catch block but not in another.

  In main.cpp you have this line:

	if (args->Sort()){ sort(file_q.begin(), file_q.end()); }

  It's quite unclear where is this 'sort()' function coming from. (And in
fact I'm perplexed that this even compiles! There's no #include <algorithm>
anywhere to find, and even if there was, there's no 'using' anywhere to
find, and the only other 'sort' I can find is a private member variable
of CL_Args.).
  You have either somehow managed to find a bug in the gcc libraries or
to make such an obfuscated piece of code that I can't figure it out... :)
  But anyways, since you are not using 'using' (which is something which
should never be used anyways, so you have a very good principle!), then if
you are using the standard sort() function it's a good idea to use 'std::sort'
for clarity. (The fact that it compiles without the include and the prefix
still perplexes me.)

  You don't need to explicitly close a std::ofstream in the destructor of
your class. std::ofstream will close itself automatically when it is
destroyed. This is not Java. ;)

  It's customary to initialize variables in the initialization list of the
constructor whenever possible (in the case of integral variables I suppose
there isn't any great advantage in doing so, but it doesn't hurt either).

  It's good programming practice to declare member functions const when
they do not change the state of the object (this makes it possible to
call these methods when having a const reference or whatever). You have
declared some of non-modifying methods const, but not all of them.

  Using namespaces is a good idea, but usually namespaces are used to
distinguish bigger wholes than just one class. Having a namespace called
'df3' containing just one class called 'DF3' sounds a bit like overdoing
things. That's not really what namespaces are useful for.

  Commenting your code is usually a good idea. It makes things easier to
understand for others.
  For example, DF3::writeDF3Header() returns an int, but it's completely
unclear what the return value means. Looking at its implementation doesn't
help either: It just returns 1. A short comment in the declaration of
this function about what the function is returning would clear things up
a lot.

  On the other hand, comments like this one do not cast vast amounts of
trust in the program:
"// for some reason, this exception isn't being unwound properly 
 // to main()'s catch block. so catch it here and exit."
  That comment means, in other words: "This is a kludge."
  (Good thing you were honest on adding such a comment, though. That's much
better than just leaving it uncommented. It's, however, usually a good thing
to resolve the problem instead of just coding a kludge around it.)

  I know it's often tempting to use C's shortcuts for some things, but
unless you are writing obfuscated code it may be a better idea to do it
the clear way, not the short way. For example, this line of code:

((r+1) == tga_readcount) ? bytes = tga_lastread : bytes = image_data.size();

takes a while to be understood. And there's absolutely no reason for doing
it that way. (And in fact, that's an expression returning a value which is
not used for anything.)
  Why not do it the clear way, which can be understood right away (and also
probably doesn't cause most compilers to warn about an unused value):

if(r+1 == tga_readcount)
    bytes = tga_lastread;
else
    bytes = image_data.size();

  And even if you wanted to use the ?: shortcut, wouldn't this be better:

byte = ((r+1) == tga_readcount) ? tga_lastread : image_data.size();


  You have hard-coded the type of the DF3 voxel: unsigned char.
  Hard-coding a type is in some cases a bad idea. This is one of such cases.
  What if the user would want to convert a 16-bit B/W TGA (or a 48-bit color
TGA) to DF3 so that voxels are 16 bits long? If you wanted to support this
you would need to make quite a lot of changes to your code.
  This is because you hard-coded the size of the voxels in your code, which
makes it rigid and hard to change.
  A much better OO design is to use an abstract voxel type which public
interface does not fix the size of the voxel to a certain small limit
(it could take brightness values as eg. longs or doubles and internally
convert them to whatever actual type is used).
  This way, even if you had fixed the type to unsigned char, you would
have done so only inside the abstract voxel type and it would be very
easy to change it to something else (eg 'unsigned short' or 'unsigned int'),
or make it dynamic altogether (ie. use the type which best matches the
input TGA pixel size and/or perhaps some command-line parameter).
  This is why abstraction is such an important OO concept.

  I'm in a hurry right now, so I only was able to make a couple of comments
about the code. Perhaps I'll review more later, if you are interested.

-- 
#macro M(A,N,D,L)plane{-z,-9pigment{mandel L*9translate N color_map{[0rgb x]
[1rgb 9]}scale<D,D*3D>*1e3}rotate y*A*8}#end M(-3<1.206434.28623>70,7)M(
-1<.7438.1795>1,20)M(1<.77595.13699>30,20)M(3<.75923.07145>80,99)// - Warp -

Post a reply to this message

"Warp" <war### [at] tagpovrayorg> wrote in message
news:415d5857@news.povray.org...
> Ross <rli### [at] everestkcnet> wrote:
> > Source code is in C++. Constructive criticism on the source is welcome.
I
> > probably over did some things and maybe blindly missed others.
>
>   1191 lines of code in 16 files for converting TGA images to DF3
> certainly feels a bit like overdoing things... :)
>   But it isn't that bad, really.

Yes, it is a bit much for a simple concept. My primary goal were threefold.
1) make a tga to df3 converter that wouldn't segfault because wrong or
invalid command line arguments and bad tga input. 2) make a tga to df3
converter that was faster than a previously available implementation. 3)
improve my C++.

I could get old versions of tga2df3 to crash just by running it from a
driectory where the files didn't exist, even if I put the full path to the
files as the command line argument. I wanted to fix that. I also wanted to
improve the speed, even though speed isn't a big deal in this program. In my
tests, i've cut the time about in half (using the "time" utility to test).
#3 is a work in progress as you have seen. I'm comming from a background
where C++ was taught as C with classes. Last year I bought Stroustrups book
and was opened to what C++ can really be. Then I bought one of Scott Meyers
book and started putting some of his tips in my code.

So, your most harsh criticism would be appreciated. if you think it's too
offtopic for here, feel free to respond wherever.

>
>   I do not understand why you need to allocate an instance of arg::CL_Args
> dynamically at the beginning of main().
>   Is there a reason why this won't do?
>
> arg::CL_Args args(argc, argv);
>
>   Unnecessarily allocating it dynamically is only a source of problems and
> has no benefits.

Yes, i realize this, tried to change it once, and something went wrong. It
was a while ago. It definately is a plan to do as you said. Your comments on
this topic of C++ in other threads was guiding me actually. I think i have a
comment where I declare arg::CL_Args that I should stop using dynamic
allocation here.

>   And in fact, you *have* one such problem in your code: There are several
> exit points in the program where the instance is not freed.

damn. thought I caught them all. that'll show me.

> If you had
> used a local instance there wouldn't be such problem. You also (quite
> inconsistently) free it in one catch block but not in another.
>
>   In main.cpp you have this line:
>
> if (args->Sort()){ sort(file_q.begin(), file_q.end()); }

yes, that should be the sort from <algorithm>. I know I am not #include-ing
it anywhere. It isn't intentionally obfuscated. i'll clean that up for sure.
So the compiler should really give a warning or not compile then? I should
really be including <algorithm> and saying "std::sort(...)". gcc glitch? i'd
blame myself before gcc though.

>
>   It's customary to initialize variables in the initialization list of the
> constructor whenever possible (in the case of integral variables I suppose
> there isn't any great advantage in doing so, but it doesn't hurt either).

i've only recently started doing this. I think I do it in a few places. I
understand the benefits and I am trying to migrate towards that. What do you
think about, i forget the proper name, exception statements on functions.
for instance saying "this function can throw exceptions of this type". like
"int foo() throws bla" or whatever. I started using it, then commented them
out. I wasn't sure if you have a heirarchy of exception classes if you would
have it throw the derived-from class, or the derived class. did that make
sense?

>
>   It's good programming practice to declare member functions const when
> they do not change the state of the object (this makes it possible to
> call these methods when having a const reference or whatever). You have
> declared some of non-modifying methods const, but not all of them.

List initialization lists, this is a part of the language I am trying to put
into my default usage. I've only used it in some cases.

>
>   Using namespaces is a good idea, but usually namespaces are used to
> distinguish bigger wholes than just one class. Having a namespace called
> 'df3' containing just one class called 'DF3' sounds a bit like overdoing
> things. That's not really what namespaces are useful for.

yeah, I wasn't quite sure what to do. it *felt* like overkill, but I didn't
know the best way to go about it. Instead of dropping namespaces altogether,
I did what I did. suggestions?


>
>   Commenting your code is usually a good idea. It makes things easier to
> understand for others.
>   For example, DF3::writeDF3Header() returns an int, but it's completely
> unclear what the return value means. Looking at its implementation doesn't
> help either: It just returns 1. A short comment in the declaration of
> this function about what the function is returning would clear things up
> a lot.
>
>   On the other hand, comments like this one do not cast vast amounts of
> trust in the program:
> "// for some reason, this exception isn't being unwound properly
>  // to main()'s catch block. so catch it here and exit."
>   That comment means, in other words: "This is a kludge."
>   (Good thing you were honest on adding such a comment, though. That's
much
> better than just leaving it uncommented. It's, however, usually a good
thing
> to resolve the problem instead of just coding a kludge around it.)
>

Commenting is an art. I'll try to improve. As far as that "kludge" comment,
it was weird. Exceptions really weren't being unwound. I don't know if it
was a compiler issue or my fault. Maybe cleaning and fixing other parts will
help make it obvious.


>   I know it's often tempting to use C's shortcuts for some things, but
> unless you are writing obfuscated code it may be a better idea to do it
> the clear way, not the short way. For example, this line of code:
>
> ((r+1) == tga_readcount) ? bytes = tga_lastread : bytes =
image_data.size();
>
> takes a while to be understood. And there's absolutely no reason for doing
> it that way. (And in fact, that's an expression returning a value which is
> not used for anything.)
>   Why not do it the clear way, which can be understood right away (and
also
> probably doesn't cause most compilers to warn about an unused value):
>
> if(r+1 == tga_readcount)
>     bytes = tga_lastread;
> else
>     bytes = image_data.size();
>
>   And even if you wanted to use the ?: shortcut, wouldn't this be better:
>
> byte = ((r+1) == tga_readcount) ? tga_lastread : image_data.size();
>

premature optimization is the root of all evil. i'm guilty here. just trying
to avoid branches.


>
>   You have hard-coded the type of the DF3 voxel: unsigned char.
>   Hard-coding a type is in some cases a bad idea. This is one of such
cases.
>   What if the user would want to convert a 16-bit B/W TGA (or a 48-bit
color
> TGA) to DF3 so that voxels are 16 bits long?

I saw you mention, i think it was you anyway, in another thread that df3's
support 8, 16, and 24(or was it 32?) bit "voxels". Is this a new thing?
Also, is the df3 format a pov-team "invention", or what? I was working off
of about 8 lines of information in the povray 3.5 documentation and for
developing a program based on a file format for the first time, it was a
little vague. i'd like to allow more for more flexibility. that wasn't a
goal for this version. i wouldn't mind implementing it now, however.

> If you wanted to support this
> you would need to make quite a lot of changes to your code.
>   This is because you hard-coded the size of the voxels in your code,
which
> makes it rigid and hard to change.
>   A much better OO design is to use an abstract voxel type which public
> interface does not fix the size of the voxel to a certain small limit
> (it could take brightness values as eg. longs or doubles and internally
> convert them to whatever actual type is used).
>   This way, even if you had fixed the type to unsigned char, you would
> have done so only inside the abstract voxel type and it would be very
> easy to change it to something else (eg 'unsigned short' or 'unsigned
int'),
> or make it dynamic altogether (ie. use the type which best matches the
> input TGA pixel size and/or perhaps some command-line parameter).
>   This is why abstraction is such an important OO concept.
>
>   I'm in a hurry right now, so I only was able to make a couple of
comments
> about the code. Perhaps I'll review more later, if you are interested.
>

I am very interested. I was fearing your comments actually. I agree with
everything you have said with respect to C++ programming in the past
(primarily resource management), and know I can't get it all right the
first, second, or third time. Like I said, feel free to comment more if you
have time. This is the first program i've ever released to public eyes,
thanks for helping.

-ross.

Post a reply to this message

Ross <rli### [at] everestkcnet> wrote:
> What do you
> think about, i forget the proper name, exception statements on functions.
> for instance saying "this function can throw exceptions of this type". like
> "int foo() throws bla" or whatever. I started using it, then commented them
> out. I wasn't sure if you have a heirarchy of exception classes if you would
> have it throw the derived-from class, or the derived class. did that make
> sense?

  If I remember correctly, the exception mechanism of C++ has been
standardized to be quite safe.
  If I'm not mistaken, you can safely throw a local temporary instance
of a class and catch-blocks catching a reference of that type (or a base
type of that class) will correctly get a reference to an existing instance,
and the instance will be destroyed at the end of that catch block.

  That is, suppose you have a "class Exception" and a
"class InvalidArgument: public Exception". You can then do
something like this:

void foo()
{
    ...
    throw(InvalidArgument());
}

and somewhere else:

try
{
    foo();
}
catch(Exception& e)
{
    ...
}

  If foo() throws an exception of type InvalidArgument, it will be
caught in the catch block above (which takes a reference of type
Exception). The reference will point to a valid instance and this
instance will be properly destroyed at the end of the catch block
(unless rethrown, of course).

  (One great thing about C++ exceptions is that the compilers can
implement them so that they do not affect the speed of the code if
exceptions are not thrown (at the cost of a bit bigger binaries, though).)

  If you are using exceptions for error handling in your program, it may
indeed be a good idea to tell for most functions if they do or do not
throw exceptions.
  Telling that a certain function does not throw exceptions is a bit like
the same kind of assertion as telling that a member function does not
modify the state of the object (with 'const'). This is not only quite
documentary, but it can also be useful sometimes.
  (The same applies, of course, to telling that a function can throw
only certain types of exceptions.)

> yeah, I wasn't quite sure what to do. it *felt* like overkill, but I didn't
> know the best way to go about it. Instead of dropping namespaces altogether,
> I did what I did. suggestions?

  Namespaces are usually useful when you are creating a library for others
to use, specially if the library will contain a numerous amount of public
names (type names, function names, etc).

  Consider the Boost set of libraries (http://www.boost.org/). All the
Boost libraries are inside a 'boost' namespace (except their implementation
of the standard libraries, which are inside the 'std' namespace, of course).
Also each individual library is located in its own sub-namespace. For
example, the lambda library (http://www.boost.org/libs/lambda/doc/) is
inside the 'lambda' namespace inside the 'boost' namespace (ie. if you
don't use 'using', you would need to use the 'boost::lambda::' prefix for
each name).
  These libraries contain quite a numerous set of things inside them,
but they seldom contain just one type.

  Anyways, my point is that usually when you are creating a big library
(which is designed to be reusable and not too specific to a certain
application), using a namespace may be a good idea.
  Using a namespace for one class inside your program may be overkill.
After all, the name of the class works almost as a namespace all by itself.

> premature optimization is the root of all evil. i'm guilty here. just trying
> to avoid branches.

  Concentrate on optimizing your algorithms and data containers (for speed
and memory usage). It's usually not necessary to optimize the code. :)

  I would say that in this case the memory optimizations take priority.
After all, the speed bottleneck in this application is disk reading and
writing, which is very slow compared to handling the data and thus the
speed of your code is not extremely relevant. The amount of memory your
data containers waste is something more important (even though in this
case it may not be very urgent either, considering how much memory there
is in current computers; as long as you keep only one image at a time
in memory there shouldn't be any problem).

  By the way, one hint regarding to disk reading and writing speed,
since you want your application to be fast:
  For several reasons the C++ streams are quite slow. Using C streams
instead can increase your application's reading and writing speed even
by a factor of two.
  (The best way of doing this is, of course, to make an abstract
reading/writing type inside which it's easy to change which type of
file reading/writing functions are used.)

> I saw you mention, i think it was you anyway, in another thread that df3's
> support 8, 16, and 24(or was it 32?) bit "voxels". Is this a new thing?

  IIRC it's a new feature of POV-Ray 3.6. See the documentation for details.

> Also, is the df3 format a pov-team "invention", or what?

  AFAIK, yes.

-- 
#macro M(A,N,D,L)plane{-z,-9pigment{mandel L*9translate N color_map{[0rgb x]
[1rgb 9]}scale<D,D*3D>*1e3}rotate y*A*8}#end M(-3<1.206434.28623>70,7)M(
-1<.7438.1795>1,20)M(1<.77595.13699>30,20)M(3<.75923.07145>80,99)// - Warp -

Post a reply to this message

Some more comments:

  - This is more a question of programming style than anything else, but
it's usually considered a good style to not to have method implementations
in the public interface of a class. (Since implementation details are
hidden at programmatical level, it's a good idea to keep them more or
less hidden at visual level as well... :) )
  If you want to define inline functions, they can be implemented at
the end of the header. It requires more writing, but the declaration
of the class keeps cleaner and nicer to look at.
  That is, something along the lines of:

class FooBar
{
 public:
    ...
    inline bool someFlag() const;
    ...

 private:
    ...
};

inline bool FooBar::someFlag() const { return theFlag; }

  (Note that the 'inline' keyword is very important in this case. Do you
know why?
  When you implement the function in its declaration inside the class the
'inline' keyword is not needed (because it's implied), but doing it this
way requires 'inline'. But why?)

  - I personally also try to make class declarations look nicer by using
whitespaces and "separator comments" to my advantage. For example something
like this:

//===============================================================
// Appropriate description
// of 'SomeClass' here
//===============================================================
class SomeClass
{
 public:
    SomeClass();
    ~SomeClass();

    void someFunction();
    void anotherFunction();




//-------------------------------------------------------
 private:
//-------------------------------------------------------
    some;
    private;
    stuff;
    here;
};

  In the implementation I also usually use visible "separators" before
functions. Also try to group function implementations by category.
I often use "//======..." separators to start a category and
"//------..." separators to start a function (if I feel the function
needs one; they don't always do). Naturally I try to write something
descriptive between these lines :)
  It would thus look something like this:

//=======================================================================
// Constructor and destructor
//=======================================================================
SomeClass::SomeClass():
    initialize(some), stuff(here)
{
}

SomeClass::~SomeClass()
{
}



//=======================================================================
// Handler functions
//=======================================================================

//---------------------------------------------------------
// Do something 1
//---------------------------------------------------------
bool SomeClass::someFunction()
{
    ....
}

//---------------------------------------------------------
// Do something 2
//---------------------------------------------------------
bool SomeClass::anotherFunction()
{
    ....
}

  These separators make it easier to find things in the code.

  - Although in most cases it doesn't matter if you return a member
string by value or by const reference, it might be a good idea to
do the latter. Some compilers might not use the copy-on-write technique
for strings, and it's a good idea for other container types.
  That is, instead of:

  std::string getName() const;

it may be a good idea to do it like:

  const std::string& getName() const;

  (Note that this can only be done for methods which return a reference
to a member string or some other string which scope is not limited to
the function itself. If you are returning a string created inside the
function then you must do it by value.)

  - A C tip (which you might already know, but...): If you have tons
of boolean flags in your class and you don't want that class to be
very big (after all, each 'bool' is 4 bytes long even though it can
only have two values), you can use the bit field property of C.
That is, instead of doing this:

  bool a, b, c, d, e, f, g;

(which takes 28 bytes in a 32-bit system), you can do this:

  int a:1, b:1, c:1, d:1, e:1, f:1, g:1;

(which takes 4 bytes).

  The 'a', 'b', etc variables will still work as if they were of type bool
(by all practical effects), but will only take 1 bit each.
  It might not be the cleanest possible way of using flags (specially if
conserving space is not an issue), but it's certainly a lot cleaner than
trying to handle the individual bits yourself (with the above syntax it
will be the compiler which will generate the appropriate bit-handling
code).

  (Note that it's safe to use 'std::vector<bool>' without having to worry
about wasted space. std::vector has a specialization for the 'bool' type
in which each value takes only 1 bit of memory.)

  - There's an easier way of putting lots of strings (or any items) inside
a vector than writing tons of push_back lines. The more the items you need
to put in the vector, the more feasible this way is:

namespace
{
    const char* names[] =
    { "-b", "-f", "-h", "--help", ...
    };
}

arg::CL_Args::CL_Args(int argc, char* argv[]):
    switches(names, names+sizeof(names)/sizeof(names[0]))
{...}

  (If you can't use the constructor of the vector to put the values
in it, you can do it afterwards with:
switches.assign(names, names+sizeof(names)/sizeof(names[0]));)

  You can use this same feature to initialize your 'arguments' array:

arg::CL_Args::CL_Args(int argc, char* argv[]):
    switches(names, names+sizeof(names)/sizeof(names[0])),
    arguments(argv, argv+argc)
{...}

  (Or if afterwards: arguments.assign(argv, argv+argc);)

  The STL is quite ingenuous and versatile. Get to know it.

  - More later.

-- 
plane{-x+y,-1pigment{bozo color_map{[0rgb x][1rgb x+y]}turbulence 1}}
sphere{0,2pigment{rgbt 1}interior{media{emission 1density{spherical
density_map{[0rgb 0][.5rgb<1,.5>][1rgb 1]}turbulence.9}}}scale
<1,1,3>hollow}text{ttf"timrom""Warp".1,0translate<-1,-.1,2>}//  - Warp -

Post a reply to this message

"Warp" <war### [at] tagpovrayorg> wrote in message
news:41616751@news.povray.org...
>   Some more comments:
>
>   - This is more a question of programming style than anything else, but
> it's usually considered a good style to not to have method implementations
> in the public interface of a class. (Since implementation details are
> hidden at programmatical level, it's a good idea to keep them more or
> less hidden at visual level as well... :) )
>   If you want to define inline functions, they can be implemented at
> the end of the header. It requires more writing, but the declaration
> of the class keeps cleaner and nicer to look at.
>   That is, something along the lines of:
>
> class FooBar
> {
>  public:
>     ...
>     inline bool someFlag() const;
>     ...
>
>  private:
>     ...
> };
>
> inline bool FooBar::someFlag() const { return theFlag; }
>
>   (Note that the 'inline' keyword is very important in this case. Do you
> know why?
>   When you implement the function in its declaration inside the class the
> 'inline' keyword is not needed (because it's implied), but doing it this
> way requires 'inline'. But why?)


besides being required in order to have the compiler consider inlining it?
no, i don't know why. i can only make a guess that it has something to do
with name mangling that the compiler performs.


>
>   - I personally also try to make class declarations look nicer by using
> whitespaces and "separator comments" to my advantage. For example
something
> like this:

:snip:

I agree, that looks nicer.


>   - There's an easier way of putting lots of strings (or any items) inside
> a vector than writing tons of push_back lines. The more the items you need
> to put in the vector, the more feasible this way is:
>
> namespace
> {
>     const char* names[] =
>     { "-b", "-f", "-h", "--help", ...
>     };
> }
>
> arg::CL_Args::CL_Args(int argc, char* argv[]):
>     switches(names, names+sizeof(names)/sizeof(names[0]))
> {...}
>
>   (If you can't use the constructor of the vector to put the values
> in it, you can do it afterwards with:
> switches.assign(names, names+sizeof(names)/sizeof(names[0]));)
>
>   You can use this same feature to initialize your 'arguments' array:
>
> arg::CL_Args::CL_Args(int argc, char* argv[]):
>     switches(names, names+sizeof(names)/sizeof(names[0])),
>     arguments(argv, argv+argc)
> {...}
>
>   (Or if afterwards: arguments.assign(argv, argv+argc);)


interesting. I knew there had to be an easier way, glad to know it now.


>
>   The STL is quite ingenuous and versatile. Get to know it.
>
>   - More later.
>

I'm trying. Thanks for the help and insight.

-ross

Post a reply to this message

"Warp" <war### [at] tagpovrayorg> wrote in message
news:41615dea@news.povray.org...

>   If I'm not mistaken, you can safely throw a local temporary instance
> of a class and catch-blocks catching a reference of that type (or a base
> type of that class) will correctly get a reference to an existing
instance,
> and the instance will be destroyed at the end of that catch block.
>
>   That is, suppose you have a "class Exception" and a
> "class InvalidArgument: public Exception". You can then do
> something like this:
>
> void foo()
> {
>     ...
>     throw(InvalidArgument());
> }
>
> and somewhere else:
>
> try
> {
>     foo();
> }
> catch(Exception& e)
> {
>     ...
> }


That matches what I understood from Stroustrup's book. He does a good job of
explaining it.


>   If you are using exceptions for error handling in your program, it may
> indeed be a good idea to tell for most functions if they do or do not
> throw exceptions.


So using the above example that you typed in, if a function throws an
InvalidArgument exception, would you say the function is throwing an
exception of type InvalidArgument or of type Exception?


> > yeah, I wasn't quite sure what to do. it *felt* like overkill, but I
didn't
> > know the best way to go about it. Instead of dropping namespaces
altogether,
> > I did what I did. suggestions?
>
>   Namespaces are usually useful when you are creating a library for others
> to use, specially if the library will contain a numerous amount of public
> names (type names, function names, etc).
>
>   Consider the Boost set of libraries (http://www.boost.org/). All the
> Boost libraries are inside a 'boost' namespace (except their
implementation
> of the standard libraries, which are inside the 'std' namespace, of
course).
> Also each individual library is located in its own sub-namespace. For
> example, the lambda library (http://www.boost.org/libs/lambda/doc/) is
> inside the 'lambda' namespace inside the 'boost' namespace (ie. if you
> don't use 'using', you would need to use the 'boost::lambda::' prefix for
> each name).
>   These libraries contain quite a numerous set of things inside them,
> but they seldom contain just one type.
>
>   Anyways, my point is that usually when you are creating a big library
> (which is designed to be reusable and not too specific to a certain
> application), using a namespace may be a good idea.
>   Using a namespace for one class inside your program may be overkill.
> After all, the name of the class works almost as a namespace all by
itself.


Yeah, I've looked at boost a little and considered using the filesytem
module just as a learning experience. I was taking the approach, though it
may not be so well implemented in my code, that df3 and tga namespaces would
be reusable libraries. that I may want to expand to a png->df3 conversion as
an excercise in understanding the png file format. however, as you say maybe
the class is sufficient in encapsulating a set of related data and
functions.


>
> > premature optimization is the root of all evil. i'm guilty here. just
trying
> > to avoid branches.
>
>   Concentrate on optimizing your algorithms and data containers (for speed
> and memory usage). It's usually not necessary to optimize the code. :)
>
>   I would say that in this case the memory optimizations take priority.
> After all, the speed bottleneck in this application is disk reading and
> writing, which is very slow compared to handling the data and thus the
> speed of your code is not extremely relevant. The amount of memory your
> data containers waste is something more important (even though in this
> case it may not be very urgent either, considering how much memory there
> is in current computers; as long as you keep only one image at a time
> in memory there shouldn't be any problem).
>
>   By the way, one hint regarding to disk reading and writing speed,
> since you want your application to be fast:
>   For several reasons the C++ streams are quite slow. Using C streams
> instead can increase your application's reading and writing speed even
> by a factor of two.
>   (The best way of doing this is, of course, to make an abstract
> reading/writing type inside which it's easy to change which type of
> file reading/writing functions are used.)


I've spent some time lurking around comp.lang.C++.moderated (if i have the
name right) and that debate seems endless. Everyone seems to say it's
implementation specific, but that in reality C I/O is faster. In my
application, i'm not even reading an entire file into memory at once. I
didn't want people to say "why the hell is this using X megabytes of
memory!" or have someone try to load a rediculously large tga file and have
it crash. I considered allowing a command line option for suggesting how big
of a buffer to use. Though I noticed that after some buffer size, speed
improvements were nonexistant. I started using Valgrind for performance
checking and memory allocation/deallocation. It's a very cool program.


>
> > I saw you mention, i think it was you anyway, in another thread that
df3's
> > support 8, 16, and 24(or was it 32?) bit "voxels". Is this a new thing?
>
>   IIRC it's a new feature of POV-Ray 3.6. See the documentation for
details.
>
> > Also, is the df3 format a pov-team "invention", or what?
>
>   AFAIK, yes.
>

Thanks again :)
ross

Post a reply to this message

Ross <rli### [at] everestkcnet> wrote:
> besides being required in order to have the compiler consider inlining it?

  Quite ironically, most compilers completely ignore the 'inline' keyword
when they evaluate whether a function is worth inlining or not. That is,
the 'inline' keyword has usually no effect whatsoever on the probability
of a function being inlined.
  In this context 'inline' is exactly as obsolete as eg. 'register' (which
compilers completely ignore).

  However, unlike 'register', 'inline' is an essential keyword, but for a
rather different reason than inlining.

  When you declare a function 'inline', you are telling the compiler
"if the implementation of this function appears in more than one object
file, the linker must merge them to one".

  Normally if the implementation of a certain function appears in two
different object files being linked to the same binary, the linker will
issue an error message: The linker can't know which one of them to use.
  However, if you had declared the functions 'inline', the compiler will
instruct the linker that both implementations are actually the same and
the linker will then use one of them.

  This is relevant when the implementation of a function is in a header
file: If the header file is included in more than one source file, this
implementation will be then placed in more than one object file.

  In C this problem was solved by defining the function 'static', which
made the function local to that object file (the same thing as is in C++
better done with a nameless namespace). However, using 'static' is not
the same as using 'inline'.
  For one, if you use 'static', the function code will be duplicated in
the final binary (increasing its size), but more importantly, functions
like this would not work properly:

static int foo()
{
    static int counter = 0;
    return ++counter;
}

  If this resides in a header file which is included in several places,
each place will get their own 'foo()' with its own counter, which might
not be what one wants.
  If, however, you do it like this (in C++):

inline int foo()
{
    static int counter = 0;
    return ++counter;
}

  Every object file calling 'foo()' will be calling one unique 'foo()',
which modifies one unique 'counter' and thus it will work as expected.

  This is why 'inline' is important.

  By the way, it's good to know that:

  - Member functions are implicitly inline if they are implemented where
    they are declared.
  - Template functions are implicitly inline.

-- 
plane{-x+y,-1pigment{bozo color_map{[0rgb x][1rgb x+y]}turbulence 1}}
sphere{0,2pigment{rgbt 1}interior{media{emission 1density{spherical
density_map{[0rgb 0][.5rgb<1,.5>][1rgb 1]}turbulence.9}}}scale
<1,1,3>hollow}text{ttf"timrom""Warp".1,0translate<-1,-.1,2>}//  - Warp -

Post a reply to this message

- In the constructor or CL_Args you are assigning "" to several member
strings. This is unnecessary. Strings naturally have proper constructors
which initialize them to empty strings. There's no need to do that yourself.
  Besides, the better way to empty a string is "outfile.erase();".

  - You have some bad type of code repetition in your command-line parsing
function. It's nothing catastrophic because the amount of command-line
switches is pretty small, but it's still not a good idea to learn bad
habits like this. It also is a potential problem if you want to expand
your program later (with more command-line options etc).
  You also use a rather inefficient way of parsing the command line, even
though it isn't anything catastrophic either because of its usual small
size (it's highly unlikely that someone will give the program a command
line which is several megabytes big). However, you shouldn't get accustomed
to bad habits here either.

  Bad repetition:
  * You have hard-coded command-line option name strings in more than one
    place.
  * You parse each one of them with a repeating similar group of code lines
    (when you find yourself doing this it should automatically make you
    wonder if you couldn't use a loop to do it more cleanly with fewer
    lines).

  You also parse the command-line arguments inefficiently: For each possible
option type you traverse the entire command line again and again.

  Possible ways of doing it in a cleaner and more efficient way are endless
but I'm not going to start pondering on this now. However, one key principle
which applies to this (as well as many other things) is abstraction.

  - A more correct (and probably efficient) way of checking whether a string
is empty or not is "name.empty()".

  - What is the ';' in this line doing? arg::ArgsError::~ArgsError(){;}

  - If a class has a constructor taking a parameter, then the class has
no default constructor (one which takes no parameters). It's not necessary
to explicitly declare one private.
  (Besides, if you disable a constructor or operator by declaring it private,
it's important that you don't implement it. If you implement it, the class
itself could use it by mistake, something you wanted to avoid in the first
place.)

  - You have a potential bug in your code: In the copy-constructor of
DataBuffer you copy the contents of the buffer given as parameter using
strcpy() (without the 'std::', by the way; probably another bug in gcc).
  strcpy() assumes the two char arrays are 0-terminated. Is this really
the case?
  The C++-way of copying things is to use std::copy() (from <algorithm>).
(If you are worrying about its performance, you probably don't need to.
Most C++ library implementations probably have a specialization for
arrays of internal types which call 'memcpy()', which is quite fast.)

  - I have written quite some programs reading TGA files myself, and
there's a rather easy (and obvious?) way of reading its header more
easily than byte by byte:
  Just use a 18-bytes long array of chars and read 18 bytes from the
input to it with one stream function call. Then you can read the bytes
you are interested in from the proper offsets in this array. (Since I'm
usually only interested in a few of them, this saves quite a lot of work.)
  So a typical code which I write to read a TGA file would look like this
(in this case I'm reading a paletted 256-color TGA):

    unsigned char header[18];
    std::fread(header, 1, 18, infile);
    if(header[2]!=1 || header[16]!=8)
    {
        std::cerr << "Only uncompressed indexed 8-bit targas, thanks."
                  << std::endl;
        return 1;
    }

    xSize = header[12]+header[13]*256;
    ySize = header[14]+header[15]*256;

    // Skip user data and palette:
    std::fseek(infile, header[0]+(header[5]+header[6]*256)*header[7]/8,
               SEEK_CUR);


-- 
#macro N(D)#if(D>99)cylinder{M()#local D=div(D,104);M().5,2pigment{rgb M()}}
N(D)#end#end#macro M()<mod(D,13)-6mod(div(D,13)8)-3,10>#end blob{
N(11117333955)N(4254934330)N(3900569407)N(7382340)N(3358)N(970)}//  - Warp -

Post a reply to this message

Ross <rli### [at] everestkcnet> wrote:
> So using the above example that you typed in, if a function throws an
> InvalidArgument exception, would you say the function is throwing an
> exception of type InvalidArgument or of type Exception?

  InvalidArgument, of course. That's what it is throwing.

-- 
#macro M(A,N,D,L)plane{-z,-9pigment{mandel L*9translate N color_map{[0rgb x]
[1rgb 9]}scale<D,D*3D>*1e3}rotate y*A*8}#end M(-3<1.206434.28623>70,7)M(
-1<.7438.1795>1,20)M(1<.77595.13699>30,20)M(3<.75923.07145>80,99)// - Warp -

Post a reply to this message