0

I have a function that translates a string coordinate to an int number from 0-9. Now the function seems to fail to get the string A10(for example) and translate it to 0,9. Maybe someone can tell me why?

Point TranslateCoordinate(char* c){
        Point newpoint;
        newpoint=malloc(sizeof(Point));//this is beacuse its an ADT
        int row=c[0];
        int column;
        newpoint->x=row-'A';
        if(c[1]== '1' && c[2]== '0'){
            newpoint->y=9;
            return newpoint;
        }
        column=c[1];
        newpoint->y=column-'1';
        return newpoint;
}

I should note that the values of the string range from 1 to 10 and from A to J. Also this is the call of the function from main; it gets a string from a file and assigns it to a struct called submarine.

while(fgets(buffer,100,fptr))
{
    if(isalpha(buffer[0]))
    {
        token=strtok(buffer,"-");
        start=TranslateCoordinate(token);
        token=strtok(NULL,"\n");
        end=TranslateCoordinate(token);
        s=Makesub(start,end,9);
        P1list=Add_to_list(s,P1list);
    }
}

The struct itself as written in the source:

struct Point_s
{
    int x;
    int y;
};

The typedef that is written in the header:

typedef struct Point_s* Point;
marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Omer Guttman
  • 141
  • 8
  • 3
    Is `newpoint=malloc(sizeof(Point));` valid? What should A10 give you , and what does it actually give you? Further how are you checking? – matt May 28 '17 at 21:04
  • @matt A10 sould give the x value of 0 and the y value of 9 i check it using a function that prints the point `printf("%d %d\n",p->x,p->y);` – Omer Guttman May 28 '17 at 21:08
  • Can you ditch the malloc line? Then tell us what you actually get when you pas 'A10'? – matt May 28 '17 at 21:10
  • 2
    `'A'-'A' = 0`. What did you expect? – Mike Nakis May 28 '17 at 21:11
  • Show **all** the code, chiefly including the definition of `Point`. Unless it is a pointer type, that assignment from `malloc()` is nonsensical. And, assuming you really need to `malloc`, then you also **really** need to `free`. Also, `collum` => `column` – underscore_d May 28 '17 at 21:12
  • You do ***not*** have "a function that translates a string coordinate to an int number from 0-9", because the function returns a `Point`, and a `Point` is not an "int number". – Mike Nakis May 28 '17 at 21:13
  • @MikeNakis yes the problem isnt with the "letter" its with the other 2 chars the that suppose to represnt a value of 10 witch will be turned into a 9 – Omer Guttman May 28 '17 at 21:14
  • the point is an ADT so from what i know malloc is needed(unless there is something i dont know) – Omer Guttman May 28 '17 at 21:16
  • 1
    What you do get from your function? Why is it wrong? Also you need to include your definition of `Point`. – matt May 28 '17 at 21:19
  • 1
    @OmerGuttman Some of us have been focusing on the problem. What do you actually get from this function? How do you define Point? – matt May 28 '17 at 21:24
  • Looks like you've now got two answers that both explain why `Point newpoint; newpoint=malloc(sizeof(Point));` is incorrect. – SiggiSv May 28 '17 at 21:37
  • 2
    **the point is an ADT** Since you access its members `x` and `y` directly in your function, it's more of a concrete data type within that function. It can still be an abstract data type outside, but in general you need some kind of abstraction barrier to call it an ADT. It may or may not be worthwhile to add this abstraction barrier in C (other languages have their own price of abstraction). It is often a reasonable strategy to make such simple data types concrete and pass them by value. – n. m. could be an AI May 28 '17 at 21:44
  • I would say not to use `typedef`d pointers to `structs` even when using an ADT. Explicitly write in the receiving functions that they are working on pointers-to-`struct`. Not doing that makes it easy to forget that you are working with a pointer and, for example, get `malloc()` wrong. Maximal clarity is far preferable. Third-party users who only see the header still see the type as abstract, because they only ever deal with pointers to it. But again, it's clearer to constantly be reminded of that. – underscore_d May 28 '17 at 21:51
  • In general a type cannot and should not be considered abstract in a function where you allocate and/or initialise an instance of it (a "constructor"). – n. m. could be an AI May 28 '17 at 22:06
  • Did the problem get resolved with either of the two answers? If you change the title of this post, to an actual description of the problem, this could be a good question. Also include the output when the program was broken. – matt May 28 '17 at 22:31
  • 1
    Note [Is it a good idea to typedef pointers?](http://stackoverflow.com/questions/750178/is-it-a-good-idea-to-typedef-pointers), to which the short answer is "No". Have you printed the values of `token` (and `buffer`) in the loop fragment? – Jonathan Leffler May 28 '17 at 22:58

2 Answers2

4
    Point newpoint;
    newpoint=malloc(sizeof(Point));

This cannot possibly be correct.

Point seems to be typedefed to be a pointer to some struct. This is by itself bad style that is going to give you problems. If you want to keep this anyway, the simplest correct calling sequence would be

    newpoint = malloc(sizeof(*newpoint));

It is also in my opinion a good all-around style even without pointer typedefs, so I would recommend using it always.

    something = malloc(sizeof(*something));

Easy to remember and doesn't need to be updated if you ever change the type of something.

I would recommend ditching that typedef and replacing it with explicit pointer notation:

typedef struct point
{
        ...
} Point;

Point* TranslateCoordinate(char* c){
    Point* newpoint;
    newpoint = malloc(sizeof(*newpoint));
    ...

The code as it stands now has undefined behaviour if sizeof(Point) is less that sizeof(*newpoint), so absolutely anything can happen.

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
  • I cannot see any other problem with this code, perhaps because there are none. If it still fails please post a [mcve]. – n. m. could be an AI May 28 '17 at 21:24
  • but i want to use an abtract data type, is there any other way i could do it? also the change from to `newpoint=malloc(sizeof(Point));` `newpoint = malloc(sizeof(*newpoint));` didnt made any noticeable diffrence (the numbers are still wrong" – Omer Guttman May 28 '17 at 21:42
  • 1
    @OmerGuttman "want to use an abstract data type" you still can. Is there some problem with it? "the numbers are still wrong" You need to post a [mcve] then. It would be very hard to find a problem without being able to see and/or reproduce it. – n. m. could be an AI May 28 '17 at 21:49
  • [here](http://coliru.stacked-crooked.com/a/c5fac2bcf287dffb)'s your code with my minimal additions. It seems to work OK. Are there any wrong numbers? – n. m. could be an AI May 28 '17 at 22:03
2

Well, it does happen to work:

#include <stdlib.h>
#include <stdio.h>

typedef struct {
    int x, y;
} *Point;

Point TranslateCoordinate(char* c){
        Point newpoint;
        newpoint=malloc(sizeof(Point));
        int row=c[0];
        int column;
        newpoint->x=row-'A';
        if(c[1]== '1' && c[2]== '0'){
            newpoint->y=9;
            return newpoint;
        }
        column=c[1];
        newpoint->y=column-'1';
        return newpoint;
}

int main (int argc, char *argv[]) {
    Point p;

    p = TranslateCoordinate("A10");
    printf("%d, %d\n", p->x, p->y);
    return 0;
}

…but only when sizeof(Point) == sizeof(void*) == 2 * sizeof(int) == sizeof(*newpoint). If that relation does not hold, anything may happen.

Thus, to avoid undefined behavior, allocate the point like this:

newpoint=malloc(sizeof(*newpoint));
marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
hidefromkgb
  • 5,834
  • 1
  • 13
  • 44
  • If, as you have assumed, `Point` is typedefd as a pointer to that struct, then what relevance do `sizeof(void*)` or `sizeof(int)` have? (not the downvoter) – underscore_d May 28 '17 at 21:16
  • @underscore_d `sizeof(Point)` allocates the exact amount of memory needed for two ints in an unpadded structure. – hidefromkgb May 28 '17 at 21:20
  • Right, the `typedef`ing as a pointer-to-unnamed-`struct` is really screwing with my grasp of reality. – underscore_d May 28 '17 at 21:24