Please wait with the 0.93 release

dpanech
2004-06-19
2004-06-23
  • dpanech

    dpanech - 2004-06-19

    John,

    I have more changes coming:
    * The old Makefiles should be removed, since they are now generated by configure (from Makefile.in); unless you have some other reason to keep them.
    * I'm adding an Automake script, it'll be much shorter and more intelligent than the current Makefile template.
    * There will be a target for building a distribution tarball ("make dist").
    * I also want to detect "uint_least64_t" (or equivalent) in configure. Once this is done, the "unsigned long longs" will have to be changed to that type.

    Also, there's a potential problem with MS Visual C++. It has a 64-bit integer type, __int64, but IIRC its iostream library truncates it to 32 bits before printing. This  happens in MSVC v6, I'm not sure about .NET. I think we may have to write our own 64-bit "<<" operator if we want to support such platforms. Shouldn't be too hard.

    Comments?
    D.

     
    • dpanech

      dpanech - 2004-06-19

      Oops, too late... I guess these changes will go in the next release then :)

       
      • John Bailey

        John Bailey - 2004-06-19

        Sorry about that. I had checked your patch over and removed the Makefiles I had made myself.  If you checked the source out of CVS and the old Makefiles were still there it was because I'd forgotten to "cvs remove" them.  After I had tested your patch on a spare PC, I thought it looked good enough for a release.

        I took a look at config.hpp.in and the resulting config.hpp on my system and started making some changes to timespan.hpp and timespan.cpp.  I'm going to finish my own changes to the other files and commit them to CVS sometime today or tomorrow.  The bulk of what I've done so far is some #if directives to typedef the proper 64-bit int type to "ulli" and just changing all occurrances of "unsigned long long int" to "ulli."  I also have added "const" to the string literal declarations as you suggested.  I also added const to the int constants' declaratioins as well.  Because I haven't finished all the changes yet, what I have on my system won't compile, but I am working on it.

        By the way, you have CVS access where you can commit your own changes directly if you want to.  The CVS instructions sourceforge has for developers has the instructions for checking out the source and committing changes (in case you've not worked with sourceforge's CVS before).  /cvsroot/timespan is where timespan's CVS directory is and the module name is timespan.

        John

         
        • John Bailey

          John Bailey - 2004-06-20

          I just committed all my changes.  I had to include inttypes.h to be able to use either uint64_t or uintmax_t.  Without doing that, g++ showed a syntax error on the typedef lines with uint64_t and uintmax_t.  If this isn't necessary for other compilers, perhaps I should wrap it with an #ifdef to determine if the compiler is g++?

          Also, I had to remove the "const" before the string literal declarations and the integer constant declarations because I kept getting linker errors (this is just a small sample from a second copy of the modified source):

          errorcheck.o(.text+0x1b5):/home/jbailey/cvs/timespan2/src/errorcheck.cpp:103: undefined reference to `monthlength'
          errorcheck.o(.text+0x1da):/home/jbailey/cvs/timespan2/src/errorcheck.cpp:123: undefined reference to `monthlengthleap'
          errorcheck.o(.text+0x236):/home/jbailey/cvs/timespan2/src/errorcheck.cpp:135: undefined reference to `monthlength'

          The same errors and more (one for each of the global arrays declared before main() in timespan.cpp) showed up in each and every .o file except timespan.o  Removing the "const" specifiers cured the problem instantly.  I did add "const" to all of the extern statements in the other .cpp files when I added it to the initial declarations, but removed it from the extern statements when i removed it from the initial declarations.

          Maybe I'll be able to sort some of it out tomorrow after I've had some sleep and more than an hour or two away from the code.

          John

           
          • dpanech

            dpanech - 2004-06-21

            Strange, I'm not getting these problems (gcc 3.3.2 / GNU ld 2.11.93). Anyhow, before we go there, there is a more serious problem:

            fileops.cpp, near line 150 (function filegetdate):

            ...
            datestruct date = { NULL, 0, 0, 0 };
            if(flags.usio && flags.usin)
              files.infile >> date.month >> ...
            ...

            It seems there is no storage behind the date.month pointer (it's NULL).

            D.

             
            • John Bailey

              John Bailey - 2004-06-23

              I've committed changes to the filegetdate function.  That's all I've touched since your commit.  I haven't yet had the chance to test it, but it seems a reasonable solution to my mind.  (I would wager $5 against my own "reason," though.)

              What I did was declare a temporary char array to receive the string out of the text file, allocate just enough memory to hold the string (and its terminating null) by using strlen and adding 1 to its return value, assigned that memory to date.month, then use strcpy to copy the string from the temporary array to date.month.  I am now making a second commit because I realize I need to cast the void pointer that malloc returns to a char pointer.  Now this is where I hit an area that I'm not entirely sure of in C++, which is do I use C-style casting, a reinterpret_cast, or a static_cast?  I'm committing a reinterpret_cast, but please correct me if I'm wrong.  I never know which type of cast to use because I use them so rarely.  If this is the proper fix, I think I may need to make it a separate function, because a quick once-over of that file shows me I might need to use the same code as many as six times.

              Thanks for pointing out the problem.  I probably would never have seen it on my own because that section is 100% my code.

              John

               
              • dpanech

                dpanech - 2004-06-23

                Malloc() is evil, don't use it in C++. Use operator "new" instead, and you won't need to cast anything:

                date.month = new char[strlen (...) + 1];
                ...
                delete[] date.month;

                Also the malloc'ed or new'ed memory should be free'd at some point (with "free()" or operator "delete"). Otherwise every time this function is called, it will leak a small chunk of memory. Probably not a big deal in most cases, unless the file you are reading from is really, really big...

                The std::string class is a lot easier to use (and thus safer) than plain char arrays.

                Another problem -- if somebody enters more than 19 characters in the temp buffer, the program will crash. The '>>' operator doesn't check array bounds, it just overflows them (but '>>' into std::string causes the string to grow as needed).

                Generally, '>>' is not suitable for parsing the input directly (neither is scanf). I'm not sure how your expected input is organized, but if its line-based the usual way to read it is:
                * read the entire line into memory as a string (with std::getline)
                * try to parse out the fields (std::istringstream)
                * if that fails print out an error message along with the name of the input file and a line number

                HTH,
                I'll look more into it when I have some more time.

                D.

                 
                • John Bailey

                  John Bailey - 2004-06-23

                  Thank you for that little nugget of info!  I didn't realize 'new' could be used on the basic types.  I thought that particular behavior of 'new' was limited to java.  That in itself is a *huge* help to know.  I was cringing as I was writing that malloc() call.

                  Knowing that, I can go back and change the code just a bit.  And give a serious look at parsing the input the way you're suggesting.

                  Again, thanks for the info about new!
                  John

                   
                  • dpanech

                    dpanech - 2004-06-23

                    No problem... I would still use std::string class though. It'll remove the need for [explicit] dynamic memory allocation and memory leaks, as well as simplify code.

                    BTW, now everything compiles in Windows/MSVC 6 (project files are under build/msvc6).

                    D.

                     
              • dpanech

                dpanech - 2004-06-23

                By the way, most of timespan code seems kind of C-ish-looking. Was it originally written in C and then converted to C++? Just curious.

                D.

                 
                • John Bailey

                  John Bailey - 2004-06-23

                  Yes, it was originally written in C, twice.  Brandon McCombs, who wrote the very first implementation of it, wrote it in C and had planned to port it to C++ himself later, but never had the time to do it.  I started writing my cleaned-up version of it in C, but wanted to take advantage of the true bool type and the default parameter capability.

                  Truthfully, much of the code looks more like C because I'm more familiar with C's libraries and much more comfortable in C.  And I knew that it was possible to "cheat" in C++ by using code that more closely resembled C than C++.

                  John

                   
      • John Bailey

        John Bailey - 2004-06-19

        I forgot to mention that I also used your "using namespace std;" suggestion in timespan.hpp.

         

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks