0

I need to return a unsigned int* from a function. The code below will compile but will crash at run time on a Windows 64 bit machine. I know I am making a silly mistake somewhere and can someone point it out for me. :p. I also have declared the function in my header, so I know its not that error.

Please note I have censored the variable names and numbers because the problem in which this function resides is not for public release yet.

Function: 

 unsigned int* convertTime(unsigned int inputInteger, unsigned short inputFrac) {
    unsigned int* output = new unsigned int[2];
    double messageTimeFraction = double(inputFrac) * 20e-6;

    output[1] = unsigned int(inputInteger + 2209032000);
    output[2] = unsigned int(messageTimeFraction * 2e32);

    return output; // Seconds
}

Implementation:

unsigned int* timeStamp;
timeStamp = convertTime(inputInteger,inputFrac);
Elpezmuerto
  • 5,391
  • 20
  • 65
  • 79
  • 5
    Note that the `unsigned int(...)` cast isn't strictly valid (Visual C++ supports it as an extension). There's [a related, somewhat technical question about this](http://stackoverflow.com/questions/2144012/explicit-type-conversion-and-multiple-simple-type-specifiers). You can just use `unsigned(...)` or you can use `(unsigned int)(...)`. – James McNellis Jun 23 '10 at 15:42
  • I hope you're calling delete[] on the ptr returned by your function, otherwise you'll have a memory leak... better yet use a structure with two ints in it – Patrick Jun 23 '10 at 16:07
  • If I remember my C++ correctly unsigned(something) is not a cast but a constructor, creating a temporary. So the code as given is definitively not C. The appropriate cast for C would be (unsigned long)(...) and for C++ it would be static_cast< unsigned long >(...) – Jens Gustedt Jun 23 '10 at 16:23
  • I believe `int` is 32 bits in Visual C++ compiling for a 64-bit app, so `unsigned int(inputFrac * 2e32)` is going to be meaningless, and almost certainly uniformly 0. This shouldn't cause a crash, but it may be an error anyway. In addition, 2209032000 is a meaningless magic number, and `messageTimeFraction` is unused. If this isn't the complete function, please let us know. – David Thornley Jun 23 '10 at 17:07

7 Answers7

8

Well, for starters you have output[1] and output[2]. Arrays are zero-indexed in c/c++, so these should be: output[0] and output[1].

But, since you're asking about c++... I urge you to use std::vector or std::pair.

(Of course, for readability's sake, you might just want to use a trivial struct with useful field names)

Stephen
  • 47,994
  • 7
  • 61
  • 70
  • Because a vector is not appropriate for representing data which happens to have two integer fields, each of which has a specific meaning (seconds and fractional). – Pete Kirkham Jun 23 '10 at 15:51
  • 4
    @Pete: Where a dynamic array is used, a `std::vector` is _always_ much, much better. If the `std::vector` is inappropriate (indeed, this should probably be a `struct` with two members), then so is the dynamic array. So you should comment on the _question_. On this answer, your down-vote is unfair and not justified. – sbi Jun 23 '10 at 15:53
  • @sbi Read the code. A dynamically allocated array (it does not change size so it is not a dynamic array) should not be used here, so replacing it with a vector is not good advice. Since it's been edited to recommend a struct, I've removed the -1. – Pete Kirkham Jun 23 '10 at 15:55
  • @Pete: Then you ought to down-vote _every answer_, since so far none advices to not to use a dynamic array. Otherwise down-voting on the second-best advice is wrong. – sbi Jun 23 '10 at 15:57
  • 1
    @Pete Kirkham : That's why i said to consider a struct or pair, passing output params is another option. On your line of thought, it's just as inappropriate to return a dynamic array of two elements... except you don't need to call `delete[]` on a vector. – Stephen Jun 23 '10 at 15:59
  • @sbi the other answers only explained what was causing undefined behaviour was, rather than making recommending changes based on the question's title rather than its content. – Pete Kirkham Jun 23 '10 at 16:01
  • @Stephen When you changed your answer to make the same recommendation mine did, I removed the down vote. – Pete Kirkham Jun 23 '10 at 16:03
  • @Pete : The OP wanted an array - who knows why? I suggested an improvement in that direction, as well as the alternative. Only the OP knows what's appropriate for his program. The recommendation of vector came from 'new int[2]` and the reference to `pair` was strictly on content. Thanks for the feedback though, and I agree that both an array and a vector are probably inappropriate. – Stephen Jun 23 '10 at 16:10
  • 1
    @Pete : Thanks for removing. By the way, just so you know... I hadn't read your answer before adding the struct reference. I've always thought it slimy when snippets from my answers end up in other people's posts. – Stephen Jun 23 '10 at 16:12
  • @Sbi Using a structure was most appropriate – Elpezmuerto Jun 23 '10 at 20:12
6

I know I am making a silly mistake somewhere and can someone point it out for me

Sure, and it has nothing to do with the Q's subject:

output[2] = unsigned int(inputFrac * 2e32);

The correct entries in output are [0] and [1] -- you're indexing out of bounds. "Undefined behavior" results (such as, the crash you observe).

Alex Martelli
  • 854,459
  • 170
  • 1,222
  • 1,395
  • It actually could have something to do with the question's subject. The code is writing on a memory location it shouldn't in the free store. Depending on how free store is implemented, it might be overwriting something important that causes a crash later. – David Thornley Jun 23 '10 at 17:00
  • @David, exactly the same could happen if the short array was allocated on stack -- it's up to the compiler to lay the storage out precisely, writing out of bounds you might overwrite the return address or something...! I.e., undefined behavior, just as for a dynamically allocated array. – Alex Martelli Jun 23 '10 at 17:53
  • Certainly, but you said it had nothing to do with the subject. That's what I was arguing against. I was supplying a possible mechanism why it might cause a crash. – David Thornley Jun 23 '10 at 18:05
  • @David, since writing to an array out of bounds can cause crashes (and many other bad things: undefined behavior!) whether the array is dynamically allocated or not, it's correct to say that the dynamically allocated nature of the array has nothing to do with it. The statement "X may happen just as likely whether Y is present or not", by ordinary Aristotelian logic, is equivalent to the statement "Y's presence has nothing to do with X's happening". – Alex Martelli Jun 23 '10 at 18:23
  • Ah, my apologies, I misread your reference to "the Q's subject". – David Thornley Jun 23 '10 at 19:25
0

The indexes in an array of 2 elements are array[0] and array[1], so change it to:

output[0] = unsigned int(inputInteger + 2209032000);
output[1] = unsigned int(inputFrac * 2e32);
Brendan Long
  • 53,280
  • 21
  • 146
  • 188
0

Arrays in C++ are zero based, so the elements of your array of size two are output[0] and output[1]

You also might want to return something that better represents the data you are returning, such as a struct with seconds and fractional_seconds members rather than creating a new array.

It's also somewhat strange what you are doing -- 2209032000 is the number of seconds in 70 years, and the result of multiplying a short by 2e32 will overflow the size of unsigned int.

Pete Kirkham
  • 48,893
  • 5
  • 92
  • 171
0

use output[0] and output[1], C / C++ arrays are 0 - based

peterchen
  • 40,917
  • 20
  • 104
  • 186
0

The more usual way to write functions like this in a C style is to pass in the reference to the variable that will be set.

As a convenience you return the output buffer so that the function can easily be used in an expression.

unsigned int* convertTime(unsigned int* output, unsigned int inputInteger, unsigned short inputFrac) {
  double messageTimeFraction = double(inputFrac) * 20e-6;

  output[0] = unsigned int(inputInteger + 2209032000);
  output[1] = unsigned int(inputFrac * 2e32);

  return output; // Seconds
}

// later
unsigned int seconds[2];
unsigned int* pseconds;
pseconds = convertTime(seconds,a,b);
Chris Becke
  • 34,244
  • 12
  • 79
  • 148
0

I created a time structure for the various formats and wrote converter functions to handle the conversions. By using structures, I do not have to worry about memory leaks and improved readability. Furthermore the code is now more scalable than using dynamic arrays, as I can add more fields and create new time formats.

struct time{
    unsigned int timeInteger;
    unsigned int timeFraction;
}time_X, time_Y;

My silly mistake was the typo of zero-based indexing but the bigger mistake was using dynamic array.

Elpezmuerto
  • 5,391
  • 20
  • 65
  • 79