5

My code is the following:

#include <iostream>
#include <sys/time.h>
using namespace std;

int main(int argc, char** argv) {
                if(argv[0])
                        argc++;

                struct timeval m_timeEnd, m_timeCreate, m_timeStart;
        long mtime, alltime, seconds, useconds;

                gettimeofday(&m_timeStart,NULL);
                sleep(3);
                gettimeofday(&m_timeCreate,NULL);
                sleep(1);

        gettimeofday(&m_timeEnd, NULL);
        seconds  = m_timeEnd.tv_sec  - m_timeStart.tv_sec;
        useconds = m_timeEnd.tv_usec - m_timeStart.tv_usec;

        mtime = (long) (((seconds) * 1000 + useconds/1000.0) + 0.5);
        seconds = useconds = 0;
        seconds  = m_timeEnd.tv_sec  - m_timeCreate.tv_sec;
        useconds = m_timeEnd.tv_usec - m_timeCreate.tv_usec;
        alltime = (long) (((seconds) * 1000 + useconds/1000.0) + 0.5);

        printf("IN=%ld ALL=%ld milsec.\n", mtime, alltime);

}

I am compiling with

g++ -W -Wall -Wno-unknown-pragmas -Wpointer-arith -Wcast-align -Wcast-qual -Wsign-compare -Wconversion -O -fno-strict-aliasing

and I have some warnings that I need to eliminate. How?

a1.cpp:21: warning: conversion to 'double' from 'long int' may alter its value
a1.cpp:21: warning: conversion to 'double' from 'long int' may alter its value
a1.cpp:25: warning: conversion to 'double' from 'long int' may alter its value
a1.cpp:25: warning: conversion to 'double' from 'long int' may alter its value
cateof
  • 6,608
  • 25
  • 79
  • 153

4 Answers4

3

If you don't really need the value rounded to the nearest millisecond - that is, if you can live with an inaccuracy of up to 1 millisecond instead of 1/2 millisecond - you can simply write

mtime = seconds * 1000 + useconds / 1000;

Otherwise, it'll have to be 

mtime = seconds * 1000 + (useconds / 500 + 1) / 2;

Edit: or not. See comment.

Mr Lister
  • 45,515
  • 15
  • 108
  • 150
  • +1: Took me a while to work out what you were doing with the second expression. You may want to add an explanation on how it works. – Martin York Feb 10 '12 at 15:58
  • @LokiAstari My formula does exactly the same as yours, only yours is simpler. Both of us, however, completely overlooked the fact that rounding off like this doesn't work on negative numbers, and useconds can be negative! So the formula actually should be `mtime = seconds * 1000 + ((usec+(usec<0 ? -500 : 500))/1000);` – Mr Lister Feb 10 '12 at 18:30
  • Are you sure. A lot of the macros associated with timeval will break if the microseconds in negative. Also it is defined as being the seconds and micro seconds from the epoch. If you allowed negative numbers in the usec the seconds portion would also be negative (as you would be counting away from the epoch in the negative direction (I think but I doubt that any of that works (or I would not trust it to work))). – Martin York Feb 10 '12 at 19:53
  • Also rounding to the nearest microsecond is silly. Just truncate the value (round towards zero) as in your first example and everything will work as expected for the user. – Martin York Feb 10 '12 at 19:55
  • @LokiAstari Yes, although the values in the timeval struct will not be negative, this `useconds` is the result of a subtraction of two longs (see the original question). And you're right, of course, about the precision, but the question was how to get rid of the warnings, not how to rewrite the calculation to make it simpler. – Mr Lister Feb 10 '12 at 20:11
1

Change it too:

mtime = seconds * 1000 + useconds/1000;

Difference is only that it is not rounding to the nearest microsecond (it rounds down)
There are no timers that are that accurate anyway.

If you really must have the extra accuracy (rounding to nearest rather than rounding to floor).

// Add 500 to useconds so that when we divide by 1000 we effectively
// round to nearest rather than truncate thus rounding to floor
mtime = seconds * 1000 + (useconds + 500) / 1000;
Martin York
  • 257,169
  • 86
  • 333
  • 562
0

This should work:

mtime = (long)(((long long)seconds*1000000 + useconds + 500)/1000);

Convert the expression for alltime in the same way.

The reason you see the warnings is that your expression converts from long to double and back to do the math. You can avoid it by re-shuffling your expressions a bit to stay entirely within integral types. Note the conversion to long long to avoid overflowing (thanks, Nick).

EDIT You can further simplify this and eliminate the conversion:

mtime = seconds*1000 + (useconds + 500)/1000;
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Isn't there a danger of this overflowing? – Nick Feb 10 '12 at 14:16
  • @Nick You are right, I added a conversion to `long long` and back to `long` to address this. Thanks! – Sergey Kalinichenko Feb 10 '12 at 14:24
  • What happens on systems where sizeof(long) == sizeof(long long). That does not protect you from overflow in the general case. – Martin York Feb 10 '12 at 15:46
  • @LokiAstari You are right, I could just move the seconds*1000 outside of the division: it cannot change the result of the integer division by 10^3 anyway, because it is multiplied by 10^6 before the addition. – Sergey Kalinichenko Feb 10 '12 at 16:07
  • 1
    Since it's C++, you should not be using C-style casts. – Mihai Todor Jun 30 '12 at 18:46
  • @MihaiTodor "C style casts" are perfectly valid part of the language that does the job. If there is nothing factually wrong with the answer, please resist the urge to downvote. – Sergey Kalinichenko Jun 30 '12 at 18:54
  • @dasblinkenlight People keep yelling all around the internet that C++ programmers should switch to the language-specific casts, in this case static_cast, because of so many reasons that I feel compelled to listen. On the other hand, if I hit some problems (and I do run into a lot of issues, since I'm rather new to C++), I keep finding examples where people still use the C-style casts. I think that we must try to offer good examples when providing answers to complete beginners, not just something that works. – Mihai Todor Jun 30 '12 at 19:17
  • @MihaiTodor While it is certainly true when it comes to pointers to objects, it does not apply at all to primitive types, which are the subject of this question. – Sergey Kalinichenko Jun 30 '12 at 19:27
  • @dasblinkenlight Even so, based on this answer http://stackoverflow.com/a/32234/1174378 I still feel that I should never ever use C-style casts in C++. – Mihai Todor Jun 30 '12 at 19:34
  • @MihaiTodor You are missing the point again: Scott's advise does not apply to primitive types: `dynamic_cast` and `static_cast` do not apply to primitives at all, and `const_cast` does not apply to this question in particular. This leaves us with `reinterpret_cast` as a theoretical possibility, and it is an extremely poor choice for this situation: it does not add clarity, and it would make the expression a lot harder to read. – Sergey Kalinichenko Jun 30 '12 at 20:11
  • 1
    I was referring to the part about making it easy to search for casts in the code. Anyway, here's an even more compelling explanation, that I've read recently: http://stackoverflow.com/a/332086/1174378 Based on this, I find your comment above rather doubtful. – Mihai Todor Jun 30 '12 at 20:27
  • @MihaiTodor The question at the link also explicitly asks about pointers, and so the answer also talks about casting pointers. Scott's recommendations regarding the search are also useful only with pointers: finding casts that are used for numeric coersion along with other C++ style casts would actually be counterproductive. – Sergey Kalinichenko Jun 30 '12 at 20:42
  • I'm still not convinced that all the other people who edited the answer that I provided left this in there by mistake: "static_cast is the first cast you should attempt to use. It does things like implicit conversions between types (such as int to float, or pointer to void*)". Writing code should be a slow process anyway, so I expect the syntax to be cumbersome sometimes, but, despite this, I think that the coding recommendations should prevail over clarity. – Mihai Todor Jun 30 '12 at 20:55
  • @MihaiTodor With the exception of performance on extremely rare occasions, there is nothing else that could possibly prevail over code clarity. Nothing kills your code base faster than the lack of clarity, because fixing bugs quickly becomes more expensive than adding new features. – Sergey Kalinichenko Jun 30 '12 at 22:57
0

The hacky way would be to cast...

mtime = (long) (((double)seconds * 1000.0 + (double)useconds/1000.0) + 0.5);

This removes any warnings...

Nim
  • 33,299
  • 2
  • 62
  • 101