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);
Improve this question
4
  • 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. You can just use unsigned(...) or you can use (unsigned int)(...). Commented Jun 23, 2010 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 Commented Jun 23, 2010 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 >(...) Commented Jun 23, 2010 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. Commented Jun 23, 2010 at 17:07

7 Answers 7

Reset to default
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)

Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

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: 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 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 : 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.
@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.
|
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).

Improve this answer

5 Comments

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, 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.
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, 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".
Ah, my apologies, I misread your reference to "the Q's subject".
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);
Improve this answer

Comments

0

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

Improve this answer

Comments

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.

Improve this answer

Comments

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);
Improve this answer

Comments

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.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.