This is the mail archive of the cygwin@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: *.cwp file recognistion patch for setup


On Sat, Sep 08, 2001 at 11:49:07PM +1000, Robert Collins wrote:
>2) Your code is very C-like.  That is, it doesn't make use of some the
>nice things C++ can do for us.  See my suggested solution below for
>somewhat more detailed approach.

I agree that the patch is C-like and I'd like to see it more C++
oriented but I wanted to point out (and I'm sure that Chuck agrees with
me) that setup itself is a strange mishmash of C/C++.  I'd like to
move away from the C-like nature of setup, though.  So if we are
implementing entirely new interfaces, it is a good idea to use
things like classes and methods rather than functions.

>3) You've got magic numbers in the code (*groan*). By this I mean that
>there are constant strings/numbers (ie gz, bz2, cwp, BZh) in the code
>with a) no explanation (and while it's trivial code at the moment, these
>things have habit of growing, and before we know it they are in much
>less readable code :}) and b) not defined for reuse. A better way to
>code that sort of thing is via #defines or enums. (ie
>#define GZ_EXT ".gz"
>#define BZ2_EXT ".bz2"
>#define CWP_EXT ".cwp"

Personally, I don't have a problem with using the strings in code since
it is obvious what you mean by ".tar.bz2" and if you do a
strncmp (ac, ".tar.bz2", 8), you have a better chance of understanding
that the 8 is the length of ".tar.bz2".  Putting the ".tar.bz2" in a
define and then using a strncmp (ac, TAR_BZ2, 8) eliminates that, although,
hmm, I guess this could also be written as
strncmp (ac, TAR_BZ2, sizeof (TAR_BZ2) - 1) .

As far as non-obvious strings, like magic numbers are concerned, I agree
100% that they should be put in a #define or constant string.

That's my 2c.  I agree with everything else that Robert said.

Thanks for taking the time to review and comment on this, Robert.

cgf

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]