Title: Comments on a Code Review

responses from the Harpist

Thank you for another thought-provoking article - 'A Big Number Class'. I have taken the opportunity of adding a few comments and queries of my own, along with an amended version of your addTo function (kjm.txt). One point worth noting is that I rarely program 'seriously' in C++ at present (Delphi being more 'suited' to my current requirements) so some of my comments, from a C++ perspective, may appear rather naïve or just strange☺

I assume that you were going to replace the pre-processor constants; you seem to have got waylaid by the original author's interesting comments.

You are probably right.

// aln.cpp
namespace {
  const int MaxString      = 1050;
  const int MaxAln         = 32700;
  const int WordLength     = 16;
  const int LongWordLength = 4;
  const int MaxNegWord     = 0x8000;
}

aln is indeed a fairly indifferent class name; whilst your suggestion of VeryLongInt seems fine, I would tend towards BigInt which is quite often used with such classes.

Yes, but it gets boring recycling other peoples names ☺

Again, the names of these functions are rather obscure. I do, however, feel that this highlights another defect, namely that the author only comments the obvious. In point of fact he appears to have misled you with his '// #define' comment. This is, I think, just a comment and not as one might suspect anything to do with conditional compilation. It's one of my pet peeves that programmers rarely comment header files. A simple one-line comment would help explain most well-written functions; even better if they came with a statement of pre- and post-conditions.

Yes, on re-reading his code I think he should have written something like:

// end of #defines

You queried why m_digits is unsigned int*; I would suspect that it points to a dynamically allocated array holding the representation. Whether it's desirable that the constructors should throw a bad_alloc is unclear (without sight of the class requirements) but it might be simpler to build the array into the class.

I would have made one of two other choices, either I would have used signed int (it has some advantages when handling intermediate results) or unsigned char (and stored a single digit per byte).

I'm uneasy about the use of static data members, esp. errnum. I'm almost certain that this will cause problems within a multi-threaded en-vironment. Using error codes instead of exceptions would require a separate errnum for each thread; doubtless thread libraries support this sort of thing, but that would render the solution (unnecessarily) implementation specific. I'm not particularly au fait with multi-threaded programming, but from my limited experience I would say that exceptions are safer and simpler than error codes here.

I think you are right.

On a first draft I'd go with the following non-explicit constructors:

  aln(long = 0);      
  aln(unsigned long); 
  aln(string const&);
  aln(aln const&); 

There may also be a case for a constructor with char const* on grounds of efficiency but I'd stick with string const& for the moment. I'd keep the non-virtual destructor as it appears aln is a concrete class.

I doubt if the class would be viable today without exceptions. Without them there is the danger of creating what are quaintly known as 'zombie objects', i.e., objects which are dead on arrival. Using error codes, client code needs to query each object after construction - aln could fail due to a lack of memory or more likely from incorrect input. Writing generic code with aln would, as a result, be problematical.

I would propose a simple exception class to replace error codes, something like aln_error perhaps:

 struct aln_error {
  errnum n;
  aln_error(errnum err) {n = err;}
};  

Public functions setting errnum all need a throw(aln_error) specifier, all others should have no throw specifiers. Additionally, the constructors need a throw(bad_alloc) since they appear to be allocating dynamic memory.

I am less happy with your exception specifiers unless you are willing to do the work to capture and process all standard exceptions that maybe thrown from functions called by the implementation code.

I'm sure I'm not the only one who would like to hear more of providing debug support in a separate class. All too many publications, if they even consider the matter, seem to assume that assert is the only debug tool available.

I think Francis recently wrote a column on this subject for EXE Magazine.

This is the one area I have a couple of qualms with your solution:

FWIW my alternative would be the member function:

  void changeSignTo(signflag sign) throw() {m_sign = sign;}

so abs becomes:

  aln abs(aln const& item) throw() {
    aln temp(item);
    temp.changeSignTo(plus);
    return temp;
  }

I agree but for a slightly different reason (the return from my version should have been by value); making this a member function results in inelegant syntax for calling it. Of course it could be a public static function (I think I prefer that version as it removes the need for a changeSignTo member function.)

At the risk of appearing ignorant or at least more ignorant than most, I'd like to solicit your opinion on the use of friends in cases like this. First let me say I'm perfectly happy forwarding public virtuals to non-member functions instead of the unnecessary use of friends. However, I wonder where the gain is when the class (such as aln appears to be) is concrete? The alternative I considered was to use a private member function forwarded by a friend.

I would turn the question round. What is the advantage of using friend declarations when a class is concrete (does not have a virtual destructor)? It is simpler to use a single consistent method. If you later decide to change the design so that the class is no longer concrete it reduces change. Every time you make a decision you have a chance of getting it wrong.

Assumptions

typos

revised function

bool addTo(IntElem* lhs, IntElem const* rhs, signed char length) throw(){
  // PURPOSE: add rep rhs[0..length-1] to lhs[0..length-1]
  // REQUIRE: length > 0 AND rhs[length-1], lhs[length-1] to exist AND
  //          all elements of lhs and rhs to be >= 0
  // PROMISE: RESULT = true on lhs[0..length-1] = old lhs[0..length-1] + rhs[0..length-1]
  //          RESULT = false on overflow, lhs[0..length-1] undefined
  assert(length > 0);

  for (int i = 0; i < length - 1; ++i) {
    lhs[i] += rhs[i];
    if (lhs[i] < 0) {
      lhs[i + 1]++;
      lhs[i] &= numeric_limits<IntElem>::max();
    } // if
  } // for

  return (lhs[length - 1] += rhs[length - 1]) >= 0;
} // addTo

Notes:

The class needs an associated numeric_limits class defined, otherwise it will be difficult to use within generic code, e.g.,

Now spotting that puts you close to C++ expert.

template<class T> void doSomething(T& n) throw(Overflow) {
  if (n < numeric_limits<T>::max()) {
    n++;
    // further processing...
  } //end if  
  else
    throw Overflow();
} // end of doSomething

A first attempt at such a class based on my underused copy of Stroustrup 3rd ed.

class numeric_limits<aln> {
public
  static const bool is_specialized = true;
  static const int digits          = 64;
  static const bool is_signed      = true;
  static const bool is_integer     = true;
  inline static aln min() throw() {return aln("-0xFFFFFFFFFFFFFFFF");}
  inline static aln max() throw() {return aln( "0xFFFFFFFFFFFFFFFF");}
};   

operator functions return by const value I seem to recall this is to prevent their use as lvalues, e.g., a + b = c should get caught be the compiler; but as a quiche-eating Delphi programmer I may well be wrong :)

You are correct.