3

Consider the following code:

char CeaserCrypt(char str[256],int key)
{
    char encrypted[256],encryptedChar;
    int currentAsci;

    encrypted[0] = '\0'; 

    for(int i = 0; i < strlen(str); i++) 
    {
        currentAsci = (int)str[i];
        encryptedChar = (char)(currentAsci+key);
        encrypted[i] = encryptedChar;
    }

    return encrypted;
}

Visual Studio 2010 gives an error because the function returns an array. What should I do?

My friend told me to change the signature to void CeaserCrypt(char str[256], char encrypted[256], int key). But I don't think that is correct. How can I get rid of the compile error?

2
  • 2
    It's interesting how many answers and how much discussion this simple question is causing. Commented Jan 17, 2011 at 19:14
  • It's a holiday in the U.S., MLK Day. Commented Jan 17, 2011 at 19:25

9 Answers 9

7

The return type should be char * but this'll only add another problem.

encrypted is "allocated" on the stack of CeaserCrypt and might not be valid when the function returns. Since encrypted would have the same length as the input, do:

int len = strlen(str);
char *encrypted = (char *) malloc(len+1);

encrypted[len] = '\0';

for (int i = 0; i < len; i++) {
// ...
}

Don't forget to deallocate the buffer later, though (with free()).

EDIT: @Yosy: don't feel obliged to just copy/paste. Use this as a pointer to improve your coding practice. Also, to satisfy criticizers: pass an already allocated pointer to your encryption routine using the above example.

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

6 Comments

It's a bad practice to allocate a buffer which have to be freed by the caller. It will probably cause memory leaks (because no one is perfect).
Geez, people. The question wasn't "how do I properly allocate/deallocate memory?".
@ssmir True enough, but given the API the OP provided, this is the simplest solution that works. Is it necessarily our jobs to rewrite his API to encourage a different style? ... <thinking about it> OK, yeah, it's probably not a bad idea to encourage better style, but that isn't what the OP asked for.
@goreSplatter: There's a difference between that and "Let's encourage the worst possible memory practices". Using basic Standard containers is the minimum.
@DeadMG I know that. I wasn't encouraging anything, though. Let alone "worst [...] practices". I feel that this question's level is this basic, that there's no need for a fully written, well revised and portable answer to suit everyone's "standards of code". This is a question about the return type of a function for pete's sake.
|
6

It wants you to return a char* rather than a char. Regardless, you shouldn't be returning a reference or a pointer to something you've created on the stack. Things allocated on the stack have a lifetime that corresponds with their scope. After the scope ends, those stack variables are allowed to go away.

Return a std::vector instead of an array.

std::vector<char> CeaserCrypt(char str[256],int key)
{
    std::vector<char> encrypted(256);
    char encryptedChar;
    int currentAsci;

    encrypted[0] = '\0'; 

    for(int i = 0; i < strlen(str); ++i) 
    {
        currentAsci = (int)str[i];
        encryptedChar = (char)(currentAsci+key);
        encrypted[i] = encryptedChar;
    }

    return encrypted;
}

There's another subtle problem there though: you're casting an integer to a character value. The max size of an int is much larger than a char, so your cast may truncate the value.

1 Comment

@DeadMG where did you find RAII here?
3

Since you're using C++ you could just use an std::string instead. But otherwise, what your friend suggested is probably best.

Comments

3

There are a few problems here. First up:

char CeaserCrypt(char str[256],int key)

As others have pointed out, your return type is incorrect. You cannot return in a single character an entire array. You could return char* but this returns a pointer to an array which will be allocated locally on the stack, and so be invalid once the stack frame is removed (after the function, basically). In English, you'll be accessing that memory address but who knows what's going to be there...

As your friend suggested, a better signature would be:

void CeaserCrypt(char* encrypted, const char str*, const size_t length ,int key)

I've added a few things - a size_t length so you can process any length string. This way, the size of str can be defined as needed. Just make sure char* encrypted is of the same size.

Then you can do:

for(int i = 0; i < length; i++) 
{
    // ...

For this to work your caller is going to need to have allocated appropriately-sized buffers of the same length, whose length you must pass in in the length parameter. Look up malloc for C. If C++, use a std::string.

3 Comments

+1 for the first part of the answer (excluding the part about overflows)
I've written the part about overflows and now can't figure out myself if I'm right or not... it's not on topic but I still want to be sure.
Stuff it, removed it until I know what I mean (it's late here).
2

If you need C compatibility make encrypted string function argument. If not, than use C++ std::string instead C style string.

And also In your code encrypted string isn't ending with '\0'

Comments

1

The problem with the original code is that you are trying to return a char* pointer (to which your local array decayed) from a function that is prototyped as one returning a char. A function cannot return arrays in C, nor in C++.

Your friend probably suggested that you change the function in such a way, that the caller is responsible for allocation the required buffer.

Do note, that the following prototypes are completely equal. You can't pass an array as a parameter to normal function.

int func(char array[256]);
int func(char* array);

OTOH, you should (if you can!) decide the language which you use. Better version of the original (in C++).

std::vector<unsigned char> CeaserCrypt(const std::string& str, const int key)
{
    std::vector<unsigned char> encrypted(str.begin(), str.end());
    for (std::vector<unsigned char>::iterator iter = vec.begin();
         iter != vec.end(); ++iter) {
        *iter += key;
    }
    return vec;
}

Do note that overflowing a signed integer causes undefined behavior.

Comments

0

VS2010 is "yelling" at you because you are trying to return a value that is allocated on the stack, and is no longer valid once your function call returns.

You have two choices: 1) Allocate memory on the heap inside your function, or 2) use memory provided to you by the caller. Number 2 is what your friend in suggesting and is a very good way to do things.

For 1, you need to call malloc() or new depending on whether you are working in C or C++. In C, I'd have the following:

char* encrypted = malloc(256 * sizeof(char));

For C++, if you don't want to use a string, try

char* encrypted = new char[256];

Edit: facepalm Sorry about the C noise, I should have looked at the question more closely and realized you are working in C++.

Comments

0

You can just do your Ceaser cipher in place, no need to pass arrays in and out.

char * CeaserCrypt(char str[256], int key) 
{     
    for(unsigned i = 0; i < strlen(str); i++)      
    {
        str[i] += key;     
    }
    return str; 
} 

As a further simplification, skip the return value.

void CeaserCrypt(char str[256], int key) 
{     
    for(unsigned i = 0; i < strlen(str); i++)      
    {
        str[i] += key;     
    }
} 

Comments

-1

well what you're returning isn't a char, but a char array. Try changing the return type to char*(char* and a char array are ostensibly the same thing for the compiler)

char* CeaserCrypt(char str[256],int key)

EDIT: as said in other posts, the encrypted array will probably not be valid after the function call. you could always do a new[] declaration for encrypted, remembering to delete[] it later on.

1 Comment

I would put another -1 if I could for the edit. It's dreadfully bad to to return pointer which have to be freed in such a way

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.