-4

I have a function that returns a pointer and I am making it to compute factorial of a given number.

This is the skeleton code which I can't alter

#include<stdio.h>
#include<string.h>
//Read only region start

char* factorial (int input1)
{

    //Read only region end
    //Write code here


}

The code I wrote is :

enter image description here

I tries this code too

#include<stdio.h>
#include<string.h>
//Read only region start

char* factorial (int input1)
{

    //Read only region end
    //Write code here
    char fact=1;
    while(input1!=0)
    {
        fact = fact * input1;
        input1--;
    }
    return fact;

}

The left side says the question and right side is the code I am writing. The error it throws is " Segmentation fault"

enter image description here

The program doesn't require main() function cause its something the mettl is using at backend to call the function and evaluate the results.

sepp2k
  • 363,768
  • 54
  • 674
  • 675
Yash Kumar Atri
  • 786
  • 1
  • 9
  • 27
  • 10
    Instead of posting images of your code, please copy paste it directly in your question – litelite Aug 08 '17 at 13:03
  • ok , give me a minute – Yash Kumar Atri Aug 08 '17 at 13:03
  • 2
    `char *fact=1;` declares a pointer and sets the pointer (address) to 1. So a reference to `*fact+input` will be a segfault since you're trying to access the address `1` in memory. You need to allocate a valid address to put in `fact`, then you can reference it properly. – lurker Aug 08 '17 at 13:06
  • It seems to me, that you declared a pointer, and assigned a value to the pointed value, without first initializing the pointer... – Usagi Miyamoto Aug 08 '17 at 13:06
  • 1
    It would be much better to post your code as text. Screenshots are hard to read and impossible to copy/paste code from. You do not have a function pointer, and it appears you also do not have an understanding of how pointers work when you dereference them. Your pointer points to an invalid address and then you write there, so it's undefined behavior what happens, but this case usually results in a crash. – Retired Ninja Aug 08 '17 at 13:07
  • 2
    Your new code is generating an integer, but then returning it as a `char *`. If the caller of that function is trying to reference that as a pointer, then you get a segfault. Your function isn't returning an address to something. It's really unclear what it's supposed to return specifically. By the way, it's not a *function pointer*. It's a *function that returns a pointer to `char`*. That pointer that it returns needs to point to a valid memory address which is available to the caller. Are you familiar with how C works? `char fact;` declares `fact` as a single byte, value range -128 to 127. – lurker Aug 08 '17 at 13:08
  • I'm going to vote to close on the basis of it being unclear what you're asking for. The segfault is easily explained (per my other comment), but you need to indicate what the caller is expecting for the `char *` if you want to know what the function should be doing. It needs to be a pointer to something. But you're currently not returning a pointer but rather a computed value. – lurker Aug 08 '17 at 13:26
  • ok wait this is the skeleton code #include #include //Read only region start char* factorial (int input1) { //Read only region end //Write code here }and I need to write the rest of the code for factorial computation , if you wanna close it just shoot a bullet to SAP Labs , cause they are the one's that gave gave this as a problem. – Yash Kumar Atri Aug 08 '17 at 13:29
  • I get, based upon the name of the function, that it computes a `factorial`. But it is returning a `char *`. Are you saying they provided no explanation as to what the return should specifically be? The only thing I can think of to map a factorial result to a `char *` is if it's asking for an ASCII string representation of the result, but I'm just guessing. You haven't provided any additional information. Are you saying they gave no information about what they want? Since you marked the posted answer as accepted, however, I assume you have the answer you were looking for. – lurker Aug 08 '17 at 13:32
  • 1
    This (https://stackoverflow.com/a/44053522/920069) answers your question, but it is written in C++. I adapted it to C here: http://ideone.com/tER5R3 If it helps, go give him an upvote. – Retired Ninja Aug 09 '17 at 02:07

1 Answers1

2

I revised your code, and here is the proper version of it:

unsigned int factorial (int input1)
{
  unsigned int res = 1;

  while(input1 > 1) {
    res = res * input1;
    input1--;
  }
  return res;
}

Your error was to return a pointer (char *) instead of a simple char (one-byte value).

I also changed char to unsigned int. Why?

  1. unsigned: because it seems you expect your result to be positive, otherwise your loop with input1-- won't work. So an unsigned allows you to have larger set of results before hitting the data type limit (max value is 127 for a signed char and 255 for an unsigned char).

  2. int: because behind the scenes (in the assembly code) the size of data passed onto the stack and into the CPU registers is an int. The ASM instructions tell to use only one byte of the register (amongs the 4 or 8 bytes depending if x32 or x64). But there is no performance gain in using a byte instead of an int whatsoever. So why not using an int if it can cover a larger set of results? :-)

  3. I also stopped the loop on the > 1 condition, because running one more iteration to just make a x1 multiplication is useless. This is a real badass optimization ... joke ;-))

UPDATE:

According to your comments, you cannot change the function definition, so this code will work by returning the result as a string (char *):

char* factorial (int input1)
{
  unsigned int res = 1;
  char *str;

  while(input1 > 1) {
    res = res * input1;
    input1--;
  }
  str = malloc(32);
  sprintf(str, "%u", res);
  return str;
}

Note that it allocates a buffer to store the results (32 should be ok even on 64 bits...). You might want to free() the buffer after use to avoid memory leaks.

Fabien
  • 4,862
  • 2
  • 19
  • 33