program taken out of book won't run

doughyi8u · 08-27-2014, 10:23 AM

The following code was taken out of a book but gets a seg fault:

Code:

//time.h
#include <cstdlib>
#include <stdio.h>

struct time {
    long secs;

    void set_time(char *);
    void get_time(char *);
};

void time::set_time(char *tod24)
{
    long hours, minutes;

    minutes=atol(&tod24[2]);//this line doesn't make sence to me
    tod24[2] =  '\0';neither does this
    hours = atol(tod24);
    secs = (hours*3600)+(minutes*60);
}

void time::get_time(char *tod)
{
    static char *tday[] = {"MIDNIGHT","P.M.","NOON","A.M."};
    int hours, minutes, amflag = 0;

    hours = secs/3600;
    minutes = (secs % 3600)/60;

    if (hours == 24) {
        hours = 0;
        amflag = (minutes == 0) ? 0 : 3;
     } else if (hours > 12) {
        hours = hours - 12;
        amflag = 1;
    } else if (hours == 12)
    amflag = 2;
    else
        amflag = 3;

        sprintf(tod,"%2d:%02d %s",hours,minutes,tday[amflag]);

}

timetest.c
#include "time.h"
#include <cstdlib>
#include <stdio.h>

int
main()
{
    char tstring[40];
    time x;

    x.secs = 23;
    x.set_time("2000");
    x.get_time(tstring);
    puts(tstring);

    return 0;
}

The comment in red above confuses me. Like I said, this is taken directly out of a book. Could someone explain how the comment in red above works or if I'm not alone on seeing it as an error

NevemTeve · 08-27-2014, 11:00 AM

Try this:

Code:

void time::set_time (const char hhmm[4])
{
    char tmp[3];
    long hours, minutes;

    memcpy (tmp, hhmm, 2);
    tmp[2]= '\0';
    hours= atoi (tmp);

    memcpy (tmp, hhmm+2, 2);
    tmp[2]= '\0';
    minutes= atoi (tmp);

    this->secs= (hours + 60*minutes)*60;
}

gdejonge · 08-27-2014, 11:06 AM

Hi,

&tod24[2] means take address of the 3th character of the string and give that to the atol function.

'\0' is the string terminator, so you cut off the string at the 3th character.

mina86 · 08-27-2014, 12:27 PM

“&tod24[2]” is equivalent to “tod24 + 2” (but some people like the awkward form

). If this still does not make sense to you, try this code:

Code:

#include <stdio.h>

int main(void) {
    const char *str = "1234";
    puts(str);
    puts(str + 1);
    puts(str + 2);
    return 0;
}

doughyi8u · 08-27-2014, 12:33 PM

I understand the programming in this (what it does) but why would I want to do that in this program? Why is it giving me a seg fault?

NevemTeve · 08-27-2014, 12:47 PM

Cause it was written by an amateur. And back then the compilers were less strict.

mina86 · 08-27-2014, 12:57 PM

Quote:

Originally Posted by doughyi8u

I understand the programming in this (what it does) but why would I want to do that in this program?

Consider this:

Code:

#include <stdio.h>

int main(void) {
  static char time[] = "2000";
  printf("minutes = %s\n", time + 2);
  time[2] = 0;
  printf("hours = %s\n", time);
  return 0;
}

Quote:

Originally Posted by doughyi8u

Why is it giving me a seg fault?

Because the fact that string literal can be assigned to a pointer to non-const char is historical legacy. Consider this instead:

Code:

int main() {
    static const char twohundredhours[] = "2000";

    char tstring[40];
    time x;

    x.secs = 23;
    x.set_time(twohundredhours);
    x.get_time(tstring);
    puts(tstring);

    return 0;
}

Notice how it does not compile even though it's, in some sense, the very same code.

PS. And yeah, similarly to what NevemTeve has said, based on the code, I would recommend you stop reading this book and use it for something useful like starting a fire place.

doughyi8u · 08-27-2014, 02:36 PM

Quote:

PS. And yeah, similarly to what NevemTeve has said, based on the code, I would recommend you stop reading this book and use it for something useful like starting a fire place.

LOL

Any comprehensive book suggestions on c++?

dugan · 08-27-2014, 02:39 PM

Quote:

Originally Posted by doughyi8u

Any comprehensive book suggestions on c++?

https://www.informit.com/store/c-plu...-9780321714114

jpollard · 08-31-2014, 12:52 PM

Quote:

Originally Posted by doughyi8u

I understand the programming in this (what it does) but why would I want to do that in this program? Why is it giving me a seg fault?

It depends on the input...

If you provided h:m format (where h is a digit for hours, and m one or more digits for minutes), starting on the third character would convert the minutes. The second part converts the hours, but assumes that the conversion will terminate when it reaches a non digit character (the ":" in my example format).

Unfortunately, it won't work correctly if you provide "h" or "hh" where "h" is a digit of the hours. This is because the third character would be at or beyond the end of the string - and hopefully only causes a segfault, because you are accessing data beyond the end of a buffer.

Looking at the entire program, I suspect the book is fairly old. It should be using snprintf to prevent formatting past the buffer, and the parameter should include the buffer length...

bulliver · 09-03-2014, 04:50 PM

Quote:

Originally Posted by dugan

https://www.informit.com/store/c-plu...-9780321714114

Yes, I have C++ Primer Plus, it is pretty decent. Bjarne's own "The C++ Programming Language" is also good, but very dense and probably not best for a beginner.

psionl0 · 09-04-2014, 01:23 AM

Quote:

Originally Posted by doughyi8u

I understand the programming in this (what it does) but why would I want to do that in this program? Why is it giving me a seg fault?

The method void time::set_time(char *tod24) modifies the character array tod24[] yet it is called with a string literal x.set_time("2000");. If that literal is rodata then it would cause a seg fault.

One way to avoid this is for set_time() to have its own character array.
eg:

Code:

void time::set_time(char *tod24)
{
    long hours, minutes;
    char hourString[4];

    minutes=atol(&tod24[2]);//this line doesn't make sence to me
/*    tod24[2] =  '\0';neither does this
    hours = atol(tod24);                  */
    hours = atol(strncpy(hourString, tod24, 2));
    secs = (hours*3600)+(minutes*60);
}

mina86 · 09-04-2014, 02:46 AM

Quote:

Originally Posted by psionl0

One way to avoid this is for set_time() to have its own character array.

At which point you also want to change the prototype of the method to:

Code:

void time::set_time(const char *tod24);