Boost CRC example program file

Question

I'm currently looking at this Boost::CRC example code which I have also inserted below.

I always try to look for suggestions for improving my own coding style when I encounter well-written and well-formatted code.

This code definitely looks like good code, but two things puzzle me about this:

• is it good practice to copy #defined constants to local const variables before using them? Why would this be a good idea?

• is it actually acceptable to just omit the parenthesis around a function body if the body is a try-catch-block?

// Boost CRC example program file ------------------------------------------//
// Copyright 2003 Daryle Walker. Use, modification, and distribution are
// subject to the Boost Software License, Version 1.0. (See accompanying file
// LICENSE_1_0.txt or a copy at <http://www.boost.org/LICENSE_1_0.txt>.)
// See <http://www.boost.org/libs/crc/> for the library's home page.
// Revision History
// 17 Jun 2003 Initial version (Daryle Walker)
#include <boost/crc.hpp> // for boost::crc_32_type
#include <cstdlib> // for EXIT_SUCCESS, EXIT_FAILURE
#include <exception> // for std::exception
#include <fstream> // for std::ifstream
#include <ios> // for std::ios_base, etc.
#include <iostream> // for std::cerr, std::cout
#include <ostream> // for std::endl
// Redefine this to change to processing buffer size
#ifndef PRIVATE_BUFFER_SIZE
#define PRIVATE_BUFFER_SIZE 1024
#endif
// Global objects
std::streamsize const buffer_size = PRIVATE_BUFFER_SIZE;
// Main program
int
main
(
 int argc,
 char const * argv[]
)
try
{
 boost::crc_32_type result;
 for ( int i = 1 ; i < argc ; ++i )
 {
 std::ifstream ifs( argv[i], std::ios_base::binary );
 if ( ifs )
 {
 do
 {
 char buffer[ buffer_size ];
 ifs.read( buffer, buffer_size );
 result.process_bytes( buffer, ifs.gcount() );
 } while ( ifs );
 }
 else
 {
 std::cerr << "Failed to open file '" << argv[i] << "'."
 << std::endl;
 }
 }
 std::cout << std::hex << std::uppercase << result.checksum() << std::endl;
 return EXIT_SUCCESS;
}
catch ( std::exception &e )
{
 std::cerr << "Found an exception with '" << e.what() << "'." << std::endl;
 return EXIT_FAILURE;
}
catch ( ... )
{
 std::cerr << "Found an unknown exception." << std::endl;
 return EXIT_FAILURE;
}

Answer 1

With the manifest constant being assigned to a const variable, you now have two ways of referring to the same value. In principle, this is superfluous, and should therefore be avoided. In practice, with manifest constants always being globally visible in C++, this is a rather moot point, but it can still be used for making a decision in lack of any other point. So, if there are any other considerations not immediately evident in the code, such as the need to be able to redefine the manifest constant AND at the same time the need to have the constant strongly typed, then by all means, it should be kept, and preferably documented.

The try statement used as a function body looks mighty weird, which means that the average C++ programmer looking at it will go "WTF?". At the same time it does not accomplish any mighty feat that will make one add "--oh, I see why it is done that way, cool!". It is okay for a coding technique to have a certain WTF factor to it, as long as it accomplishes something neat, the neatness of which is proportional to its WTFness, so as to justify it. All that the try-statement-used-as-a-function-body accomplishes is to spare us from having to type an additional --but expected-- pair of curly brackets. Therefore, it should definitely be avoided.

• Felix Dombek: I think the intention for the try block as function body was that it's immediately evident that no code can be put somewhere else than in the try block, meaning that there can be no unhandled exceptions. But, of course, the catch block could, in theory, still throw ...
• Mike Nakis: Right, it could still throw. Also, it could fail to catch some exceptions; you don't know unless you scroll down to the catch statements and make sure that one of them is indeed a catch-all.

Answer 2

is it good practice to copy #defined constants to local const variables before using them? Why would this be a good idea?

It's not good or bad, but it's definitely weird. In general I prefer constant values to #defines because they have an explicit type that can be checked at compile time. The only benefit (and I hesitate to call it that) of this code is that you could define PRIVATE_BUFFER_SIZE in another header file and it would override the definition here.

is it actually acceptable to just omit the parenthesis around a function body if the body is a try-catch-block?

It might compile, but it's a terrible practice. Don't do it! ;)

• Winston Ewert: The missing braces around main irks me, but I can't express a reason why it seems like a bad idea. I suspect the define trick is used in order to allow it to be specified as a compile time constant on the command line.
• Kerrek SB: This is called a "function-try-block". It's a somewhat unusual feature (and generally only aimed at constructors), so some people object to it. It has to follow certain special rules, so perhaps it's less confusion to avoid those.

Answer 3

I'd invert the condition and use a continue if there is an error with the stream:

for (int i = 1; i < argc; ++i) {
 std::ifstream ifs(argv[i], std::ios_base::binary);
 if (!ifs) {
 std::cerr << "Failed to open file '" << argv[i] << "'." << std::endl;
 continue; // or return EXIT_FAILURE;
 }
 do {
 char buffer[buffer_size];
 ifs.read(buffer, buffer_size);
 result.process_bytes(buffer, ifs.gcount());
 } while (ifs);
}

http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

I'm not a C++ master, but maybe the streams should be closed somewhere.

(I know that this isn't an answer to your questions but maybe you find it useful.)

• Nathanael: Strictly speaking yes, ifstream ifs should be closed. Practically speaking, in an application of this size it doesn't matter. The streams will be destroyed when the application terminates.
• Winston Ewert: @Nathanael, eh? This is C++, the file will be closed when the ifs object is destructed at the end of the loop body. Each file will be closed before the next one is opened.
• Nathanael: @Winston Ewert: You're correct. Too much time spent in other languages for me.