PDA

View Full Version : Best way to convert




Miglu
Oct 19, 2010, 03:23 PM
What is the best way to convert tv_sec, which is of type time_t, into a C string? There is no formatter for it in sprintf. I tried to use strftime(s, 5, "%S", tp->tv_sec) but got "warning: passing argument 4 of 'strftime' makes pointer from integer without a cast".



lee1210
Oct 19, 2010, 03:45 PM
It's likely that time_t is going to end up being an int of some variety. Are you trying to just display the number of seconds that it holds, or do you want a nicely formatted date that this value represents (i.e. second since epoch -> date and time represented)?

strftime's 4th argument is of type struct tm *. Passing in an int to this parameter is not going to go well. What type is tp? Is it a struct tm *? If so, you can pass that to strftime.

-Lee

Miglu
Oct 19, 2010, 04:02 PM
I just want the number of seconds.
tp is of type struct timespec.
What is the best way to convert time_t to a string?

iSee
Oct 19, 2010, 04:13 PM
Well, if you're concerned about maximum cross-platform compatibilty, you'll have to do your own research, but time_t is (always? -- check C++ spec for that) an integer time of some size. So your probably safe casting it to a long long int and using the appropriate printf format specifier for that. E.g.,


// where t is of type time_t
printf("t is: %lld\n", (long long) t);

Miglu
Oct 19, 2010, 04:23 PM
I am using Objective-C, not C++. I did snprintf(string, 4, "t is: %lld\n", (long long)tp->tv_sec), but got EXC_BAD_ACCESS. What is the problem?

chown33
Oct 19, 2010, 05:12 PM
I am using Objective-C, not C++. I did snprintf(string, 4, "t is: %lld\n", (long long)tpc->tv_sec), but got EXC_BAD_ACCESS. What is the problem?
Post complete code.

What is the type of string? Show the actual declaration you have in your code.

Specifying a length of 4 is clearly wrong, because the "t is: " part is longer than 4.

Did you look at the reference doc for snprintf()? Do you understand what every one of the parameters means?


Since you're using Objective-C, have you looked at the init methods of NSString? For example, the ones that take a format-string?

Miglu
Oct 19, 2010, 05:13 PM
Whatever I cast tp->tv_sec to, I get EXC_BAD_ACCESS. What is the problem?

lee1210
Oct 19, 2010, 07:25 PM
Give us code we can compile that crashes. We have no idea what's wrong right now.

-Lee

iSee
Oct 20, 2010, 08:23 AM
I am using Objective-C, not C++. I did snprintf(string, 4, "t is: %lld\n", (long long)tp->tv_sec), but got EXC_BAD_ACCESS. What is the problem?

I believe my suggestion should work for C and Objective-C, too.
As was mentioned,

snprintf(string, 4, "t is: %lld\n", (long long)tp->tv_sec)

is wrong because of the 4.
Also check that string is a character buffer of sufficient size to hold the result.
Also, ensure tp is correctly initialized and isn't null. You could use the debugger to help with that.

Sydde
Oct 20, 2010, 08:45 AM
Whatever I cast tp->tv_sec to, I get EXC_BAD_ACCESS. What is the problem?

Is tp a struct or a pointer to a struct?

subsonix
Oct 20, 2010, 09:15 AM
What is the best way to convert tv_sec, which is of type time_t, into a C string? There is no formatter for it in sprintf. I tried to use strftime(s, 5, "%S", tp->tv_sec) but got "warning: passing argument 4 of 'strftime' makes pointer from integer without a cast".

Your just suppose to pass *tp not tp->tv_sec.


size_t strftime(char *restrict s, size_t maxsize, const char *restrict format, const struct tm *restrict timeptr);

Miglu
Oct 20, 2010, 09:39 AM
I understand snprintf's arguments. Whatever the second argument is, I get EXC_BAD_ACCESS.


//This is in the header
struct timespec *tpc;

//This is in the implementation
- (void) mach_absolute_differenceEnd: (uint64_t) end Start: (uint64_t) start {

uint64_t difference = end - start;
static mach_timebase_info_data_t info = {0,0};

if (info.denom == 0)
mach_timebase_info(&info);

uint64_t elapsednano = difference * (info.numer / info.denom);

tpc->tv_sec = elapsednano * 1e-9;
tpc->tv_nsec = elapsednano - (tpc->tv_sec * 1e9);
}

//This is in a method in the implementation
char *string;
snprintf(string, 4, "t is: %lld\n", (long long)tpc->tv_sec);

subsonix
Oct 20, 2010, 09:50 AM
Yeah, but if you got room in string and use the correct format specifier you can convert a time_t variable with snprintf.


#include <time.h>
#include <stdio.h>

int main()
{
time_t t = 123;
size_t bufsize = 128;
char buf[bufsize];

snprintf(buf, bufsize, "%lu", t);
puts(buf);


return 0;
}

subsonix
Oct 20, 2010, 10:02 AM
I understand snprintf's arguments.


char *string; // this is a pointer to nowhere
snprintf(string, 4, "t is: %lld\n", (long long)tpc->tv_sec);

You can't just write 4 bytes to nowhere, you need to use malloc/calloc, or set aside some memory on the stack using an array declaration.

lee1210
Oct 20, 2010, 10:18 AM
I think the real lesson here is:
Not posting code == 16 hours with people guessing at your problem
Posting code == Answer with exactly what's wrong in about 20 minutes

It would have been even faster if you could have given something to compile like:

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

int main(int argc, char *argv[]) {
struct timespec *tpc;
char *string;
tpc = calloc(1,sizeof(struct timespec));
tpc->tv_sec = 234;

snprintf(string,4,"t is: %lld\n",(long long)tpc->tv_sec);
return 0;
}


The 4 would still be wrong, but we'd been able to see right away what was going on.

-Lee

jared_kipe
Oct 20, 2010, 10:50 AM
I think the real lesson here is:
Not posting code == 16 hours with people guessing at your problem
Posting code == Answer with exactly what's wrong in about 20 minutes

It would have been even faster if you could have given something to compile like:

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

int main(int argc, char *argv[]) {
struct timespec *tpc;
char *string;
tpc = calloc(1,sizeof(struct timespec));
tpc->tv_sec = 234;

snprintf(string,4,"t is: %lld\n",(long long)tpc->tv_sec);
return 0;
}


The 4 would still be wrong, but we'd been able to see right away what was going on.

-Lee

Yeah but if he had made the above code he might have figured it out on his own, and where is the fun in that?

EDIT:
Every time I'm having a problem with some kind of weird crash or memory leak **, before setting up a new thread about the problem I made a super simple test case to try to narrow down the problem.

** (my latest one came from the fact that in iOS sends (applicationWillResignActive:) both when locking and when multi-task switching out of an app, but only sends (applicationWillEnterForeground:) when coming back from multi-tasking and NOT from locking causing a seemingly very random crash with the ridiculous code I have setup for iAds to not be showing when there are no ads to display)

Miglu
Oct 20, 2010, 04:41 PM
Thanks. Now I am going to ask an offtopic question but it is not significant enough to deserve its own thread: char string[100];
fscanf(fp, "%s", string);
printf("%s", string);
I am trying to get a list of 7-digit floats separated by ; from the file that is pointed to by fp. However, only the first one and the ; after it is printed. What is the problem?

lee1210
Oct 20, 2010, 04:51 PM
We can't compile that code, but it's a start. Post a copy of the data file you're using. I tried the code below with the data file below and it worked fine (read the whole line).


#include <stdio.h>

int main(int argc, char *argv[]) {
char string[100];
FILE * fp;
fp = fopen("floats.txt","r");
fscanf(fp, "%s", string);
printf("%s\n", string);
}


floats.txt

1234567.0;9876543.0;12345.3


-Lee

Miglu
Oct 20, 2010, 04:59 PM
I tried this: 0.258800; 0.000000; -0.965900;

chown33
Oct 20, 2010, 05:22 PM
I tried this: 0.258800; 0.000000; -0.965900;

Read fscanf's man page. fscanf's %s stops at whitespace.

Your data has whitespace after the semicolon and before the next number.

So fscanf works correctly. The problem is you don't fully understand how it works.

lee1210
Oct 20, 2010, 05:23 PM
From the fscanf manpage:

s Matches a sequence of non-white-space characters; the next pointer must be a pointer to char, and the array must be
large enough to accept all the sequence and the terminating NUL character. The input string stops at white space or at
the maximum field width, whichever occurs first.

If an l qualifier is present, the next pointer must be a pointer to wchar_t, into which the input will be placed after
conversion by mbrtowc(3).


So why might it have stopped at the first space?

-Lee

Miglu
Oct 21, 2010, 10:20 AM
How to get the strings after the whitespaces?

jared_kipe
Oct 21, 2010, 01:12 PM
How to get the strings after the whitespaces?

*spits coffee out of mouth*

Seriously? You do it again. You keep calling fscanf over and over until fscanf gets exhausted and refuses to work for you anymore.

Miglu
Oct 21, 2010, 02:51 PM
Now I have ; stuck to every string. What is the best way to get it off? I tried string = strsep(&string, ";"), but I get "Incompatible types in assignment".

lee1210
Oct 21, 2010, 03:02 PM
Now I have ; stuck to every string. What is the best way to get it off? I tried string = strsep(&string, ";"), but I get "Incompatible types in assignment".

That isn't compilable code. If you post compilable code we can help you. Chances are string is declared:
char string[100];
Which is not assignment-compatible. In this case I don't see a need to do anything with the result of strsep, but you could assign it to a temporary char * if you needed to do some comparison.

-Lee

Miglu
Oct 21, 2010, 03:13 PM
"The strsep() function locates, in the string referenced by *stringp, the first occurrence of any character in the string delim (or the terminating `\0' character) and replaces it with a `\0'. The location of the next character after the delimiter character (or NULL, if the end of the string was reached) is stored in *stringp. The original value of *stringp is returned."

Is not the "original value of *stringp" what I am trying to get? If it is not, what is the purpose of strsep?

lee1210
Oct 21, 2010, 03:28 PM
Look at the example here:
http://www.delorie.com/djgpp/doc/libc/libc_773.html

It is used to "iterate" through the tokens of a string. That is not what you want to do here. It might be easier to do:

if(strlen(string) > 0) {
if(string[strlen(string)-1] == ';') string[strlen(string)-1] = '\0';
}


It may be helpful to explain to us in more detail what this project is doing, and what you're hoping to achieve, as well as the level of programming proficiency you have. If you are not at the point that you understand what man pages are saying, we need to know that to give you a more thorough explanation. If this is a school project, you need to say so so we don't do your homework for you. If this is just to "get it to work" we will guide you differently than if you are doing this for practice to learn how to code or a new language.

-Lee

Miglu
Oct 21, 2010, 03:45 PM
This is in the description: "replaces it with a `\0'". What is the purpose of this if the "the location of the next character after the delimiter character is stored in *stringp"? So that the original string, that has the delimiter, is returned, and *stringp's original value is overwritten with the location of the next string after the delimiter. Where does the string with the \0 in place of the delimiter go?

lee1210
Oct 21, 2010, 03:56 PM
The return value will be to the base of the original string, which will now be terminated at the point where the first delimiter was found. *stringp has the address of the character right after that delimiter, basically "the rest" of the string. When called again the next delimiter is found, replaced with '\0', and *stringp is "advanced" again to the position immediately following the delimiter. This allows you to advance through a string. Compile and run this, and observe the output.


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

int main(int argc, char *argv[]) {
char *string,*tmp;
string = calloc(200,sizeof(char));
strcpy(string,"1234.32;12343.43;12324.4;3235.3");
printf("Original address of string: %p\n",string);
char **strptr = &string;
while(tmp = strsep(strptr,";")) {
printf("string: %p :: %s\n",string,string);
printf("tmp: %p :: %s\n\n",tmp,tmp);
}
}


-Lee

Miglu
Oct 21, 2010, 04:04 PM
The description just says "The original value of *stringp is returned", not "The base of the original string which will now be terminated at the point where the first delimiter was found is returned", which it should.
Anyway, if that is the case, why can
char string[10];
string = strsep(NULL, ";"); not be used to remove the delimiter? It produces "incompatible types in assignment".

lee1210
Oct 21, 2010, 04:11 PM
They are the same. The man page might be more concise, but the meaning is the same.

You cannot assign to a statically allocated array. strsep returns a char *, string is a char[]. Think of it like this:

int main(int argc, char *argv[]) {
char string[100];
string = "1234";
}


You get the same error when trying to compile.

Did you run the code I posted? If not it might be instructive to do so. Seeing what's happening with the various pointers involved might help you understand why what you're trying is failing. I also posted code above that will work fine for chomping off trailing ';'s. You could also write a very short function to trim off the last character of any string if you find you need to do so often.

-Lee

chown33
Oct 21, 2010, 04:13 PM
Anyway, if that is the case, why can
char string[10];
string = strsep(NULL, ";"); not be used to remove the delimiter? It produces "incompatible types in assignment".

That code won't compile because you can't assign another value to string. Declared arrays cannot be reassigned. They are effectively constants.

Also, passing NULL as the first argument to strsep has no possibility of working. It simply makes no sense at all.

Considering all the posts you've made in this thread, you seem to have an inadequate understanding of pointers. That's the one underlying theme that ties together every problem you've posted. Until you address this fundamental problem, you will continue to have problems whenever pointers are involved.

Miglu
Oct 21, 2010, 04:18 PM
I got the desired result using if(strlen(string) > 0) {
if(string[strlen(string)-1] == ';') string[strlen(string)-1] = '\0';
}However, I want to know why using strsep does not work.
Using char *string instead of char string[10] helped, as it removed the error, but now I get EXC_BAD_ACCESS. What is the problem? char *string;
fscanf(fp, "%s", string);
string = strsep(&string, ";"); So neither char *string or char string[] works, as both produce errors.

lee1210
Oct 21, 2010, 04:34 PM
What is the problem?

As chown33 stated, the problem is that you don't understand pointers.

The technical problems with that code (which I still can't compile) are:
string is an uninitialized pointer. There is not usable memory on the other end of it to read into.

&string will work if it points to a char * that is modifiable, which points to memory which is modifiable.

strsep is by no means the right way to remove a character from the end of a string. I posted 3 lines of code above that will check if the last character of a string is a ; and will replace it with '\0' if it is.

-Lee

Edit: in your edit you added that you tried the code I posted above. It works because it suits the problem, while strsep does not. It iterates through a string modifying the contents and the pointer passed in to facilitate doing so. Did you run the strsep code I posted to see how it works?

Miglu
Oct 21, 2010, 04:43 PM
Thanks. I actually understand pointers, but I did not understand strsep and declared arrays properly. Could you also answer this question: I have a list of floats in a text file and want to get them one line at a time. fscanf stops at whitespaces so how to know when there is a carriage return?

Miglu
Oct 22, 2010, 04:34 AM
I tried to use getline but it does not seem to be supported by gcc.

subsonix
Oct 22, 2010, 04:58 AM
I tried to use getline but it does not seem to be supported by gcc.

getline is a c++ function to use it you can do like this:


#include <iostream>

int main()
{
char str[256];
std::cin.getline(str, 256);
std::cout << str << std::endl;

return 0;
}



But for a c equivalent use fgets. The only anoying thing about fgets is that you need to strip of the newline as it becomes part of the string.


#include <stdio.h>

int main()
{
char str[256];
fgets(str, 256, stdin);
puts(str);

return 0;
}

lee1210
Oct 22, 2010, 07:29 AM
The only anoying thing about fgets is that you need to strip of the newline as it becomes part of the string.

The good news is code has already been provided in this thread to strip the last character of a string. It was inefficient due to two strlen calls, but that's an easy fix, and it would be easy to wrap in a function.

There is a C getline implementation, but it was added in FreeBSD after the most recent version adapted for OS X. Alternate implementations are available online if desired.

-Lee

subsonix
Oct 22, 2010, 09:05 AM
The good news is code has already been provided in this thread to strip the last character of a string. It was inefficient due to two strlen calls, but that's an easy fix, and it would be easy to wrap in a function.


I agree, and in the case of using fgets on a file as requested in post #35 this is not a problem with, for example atof() that can convert the string regardless of nl.

Miglu
Oct 22, 2010, 12:45 PM
Thanks.

It was inefficient due to two strlen calls, but that's an easy fix, and it would be easy to wrap in a function. Could you show an efficient way.

Another problem: I use this method to get coordinates from an arbitrary position in a list of them from a text file:
- (void) getCoords: (int) i Array: (float **) a {
char *line = (char *) malloc(50);
for (int j = 0; j < i + 1; j++) {
line = fgets(line, 50, fp);
}
char *x, *y, *z;
x = (char*) malloc(20);
y = (char*) malloc(20);
z = (char*) malloc(20);

x = strsep(&line, ";");
y = strsep(&line, ";");
z = strsep(&line, ";");

float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
} I print them as they come. However, only the first 32 coordinates are printed, then I get EXC_BAD_ACCESS at strtodg, which is in strtof's stack. What is the problem?

lee1210
Oct 22, 2010, 01:00 PM
void chompChar (char *input) {
int len = strlen(input);
if(len > 0) input[len-1] = '\0';
}

This chomps the last character from any string. Adapting it to accept a character or list of characters to be chopped would be trivial.

As for that code... You are leaking all of the memory you malloc, so that's no good. a is float ** and coords is float[] (or float *). coords is on the stack, and you return a pointer to it, which is wrong. Is it the 32nd line that has the problem? So the 32nd call to this function? What are the arguments to atof when the crash occurs?

-Lee

Edit: oops, just saw *a. Still, returning a pointer to coords like that is a bad time.

chown33
Oct 22, 2010, 01:01 PM
Post the input data.

Post the stack trace from the crash.

Learn to use the debugger. When your program crashes, use the debugger to examine the data of the line that was read, the converted values, etc. to determine where the error is.

In all seriousness, if your only debugging strategy is to ask someone else to debug your code for you, then you really need to improve your skill set. This bug seems like a good one to learn the debugger on. You have a crash, it's consistent (same place), and it's isolated to a single method or function. It's almost perfect as a non-trivial but apparently isolated bug.

Oh, and all those mallocs in your method are leaking memory.

Miglu
Oct 22, 2010, 01:09 PM
all those mallocs in your method are leaking memory. What should I use instead of them?

Asking someone else to debug is not my only strategy. My strategies are using printf and looking at the call stack.

lee1210
Oct 22, 2010, 01:15 PM
What should I use instead of them?

Asking someone else to debug is not my only strategy. My strategies are using printf and looking at the call stack.

x,y,z don't need to have memory allocated at all. Are you positive you understand pointers? You are assigning a pointer to some new memory to those variables, then immediately assign to them, losing the original pointer. You could never free the memory at that point. As for line, you need to call free on its pointer before you leave the method and lose the pointer.

The debugger must become part of your repertoire. printf can only take you so far, and you don't always have the luxury of endlessly editing production code.

-Lee

Miglu
Oct 22, 2010, 01:24 PM
Doing free(&line) causes SIGABRT and this to be printed to the console: "ModelTest(6518,0x7fff7116ec20) malloc: *** error for object 0x7fff5fbff090: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug".

lee1210
Oct 22, 2010, 01:28 PM
Doing free(&line) causes SIGABRT and this to be printed to the console: "ModelTest(6518,0x7fff7116ec20) malloc: *** error for object 0x7fff5fbff090: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug".

&line is the address where the char * line is stored, so it is char **. The storage for line is on the stack. The pointer it is storing was returned from malloc. This is what needs to be passed to free.

-Lee

Edit: by the way, did you ever compile and run my strsep example? tmp never has malloc run in that code. The print statements show you all the pointers involved, which might help in seeing what it's actually doing.

chown33
Oct 22, 2010, 01:29 PM
My strategies are using printf and looking at the call stack.

Then where is your debugging output?

Where is the crash call-stack (stack trace) I asked for?

Where is the data that causes the crash?

If those are your debugging strategy, then anyone else who you ask to debug your code will need to know those same things, especially the stack trace and the data that causes a crash.


Have you noticed that you perform no error checking at all in your code? You don't check for null from gets(), you don't check that strsep finds a ;, you don't check that the conversion works.

Miglu
Oct 22, 2010, 01:45 PM
Call stack: #0 0x7fff88c4fbeb in __strtodg
#1 0x7fff88c4fac3 in strtof_l
#2 0x10000209d in -[OpenGLView getCoords:Array:] at OpenGLView.m:177
#3 0x1000018bb in -[OpenGLView initWithFrame:] at OpenGLView.m:68
#4 0x7fff803aa198 in -[NSCustomView nibInstantiate]
#5 0x7fff80302263 in -[NSIBObjectData instantiateObject:]
#6 0x7fff8030164e in -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:]
#7 0x7fff802ffcd9 in loadNib
#8 0x7fff802ff1e9 in +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZone:ownerBundle:]
#9 0x7fff802ff021 in +[NSBundle(NSNibLoading) loadNibNamed:owner:]
#10 0x7fff802fc5a3 in NSApplicationMain
#11 0x100001399 in main at main.m:13


strtodg:
0x00007fff88c4fb4a <+0000> push %rbp
0x00007fff88c4fb4b <+0001> mov %rsp,%rbp
0x00007fff88c4fb4e <+0004> push %r15
0x00007fff88c4fb50 <+0006> push %r14
0x00007fff88c4fb52 <+0008> push %r13
0x00007fff88c4fb54 <+0010> push %r12
0x00007fff88c4fb56 <+0012> push %rbx
0x00007fff88c4fb57 <+0013> sub $0x128,%rsp
0x00007fff88c4fb5e <+0020> mov %rdi,%r12
0x00007fff88c4fb61 <+0023> mov %rsi,-0x100(%rbp)
0x00007fff88c4fb68 <+0030> mov %rdx,-0x108(%rbp)
0x00007fff88c4fb6f <+0037> mov %rcx,-0x110(%rbp)
0x00007fff88c4fb76 <+0044> mov %r8,-0x118(%rbp)
0x00007fff88c4fb7d <+0051> mov %r9,%rbx
0x00007fff88c4fb80 <+0054> test %r9,%r9
0x00007fff88c4fb83 <+0057> jne 0x7fff88c4fb91 <__strtodg+71>
0x00007fff88c4fb85 <+0059> mov -0x17af9aec(%rip),%rax # 0x7fff711560a0
0x00007fff88c4fb8c <+0066> mov (%rax),%rbx
0x00007fff88c4fb8f <+0069> jmp 0x7fff88c4fba0 <__strtodg+86>
0x00007fff88c4fb91 <+0071> cmp $0xffffffffffffffff,%r9
0x00007fff88c4fb95 <+0075> lea -0x17af31dc(%rip),%rax # 0x7fff7115c9c0 <__global_locale>
0x00007fff88c4fb9c <+0082> cmove %rax,%rbx
0x00007fff88c4fba0 <+0086> mov %rbx,%rdi
0x00007fff88c4fba3 <+0089> callq 0x7fff88d2a8c4 <dyld_stub_localeconv_l>
0x00007fff88c4fba8 <+0094> mov (%rax),%rax
0x00007fff88c4fbab <+0097> mov %rax,-0x88(%rbp)
0x00007fff88c4fbb2 <+0104> movl $0x0,-0x3c(%rbp)
0x00007fff88c4fbb9 <+0111> lea -0x60(%rbp),%r13
0x00007fff88c4fbbd <+0115> movq $0x0,-0x60(%rbp)
0x00007fff88c4fbc5 <+0123> movq $0x0,-0x68(%rbp)
0x00007fff88c4fbcd <+0131> mov -0x108(%rbp),%rax
0x00007fff88c4fbd4 <+0138> mov (%rax),%eax
0x00007fff88c4fbd6 <+0140> mov %eax,-0xd4(%rbp)
0x00007fff88c4fbdc <+0146> mov %r12,-0x50(%rbp)
0x00007fff88c4fbe0 <+0150> lea 0x19(%rip),%rdx # 0x7fff88c4fc00 <__strtodg+182>
0x00007fff88c4fbe7 <+0157> mov -0x50(%rbp),%rdi
0x00007fff88c4fbeb <+0161> cmpb $0x2d,(%rdi) //The problem is in this line.
0x00007fff88c4fbee <+0164> ja 0x7fff88c4fcf2 <__strtodg+424>
0x00007fff88c4fbf4 <+0170> movzbl (%rdi),%eax
0x00007fff88c4fbf7 <+0173> movslq (%rdx,%rax,4),%rax
0x00007fff88c4fbfb <+0177> add %rdx,%rax
0x00007fff88c4fbfe <+0180> jmpq *%rax
0x00007fff88c4fc00 <+0182> faddl (%rax)
0x00007fff88c4fc02 <+0184> add %al,(%rax)
0x00007fff88c4fc04 <+0186> repnz add %al,(%rax)
0x00007fff88c4fc07 <+0189> add %dh,%dl
0x00007fff88c4fc09 <+0191> add %al,(%rax)
0x00007fff88c4fc0b <+0193> add %dh,%dl
0x00007fff88c4fc0d <+0195> add %al,(%rax)
0x00007fff88c4fc0f <+0197> add %dh,%dl
0x00007fff88c4fc11 <+0199> add %al,(%rax)
0x00007fff88c4fc13 <+0201> add %dh,%dl
0x00007fff88c4fc15 <+0203> add %al,(%rax)
0x00007fff88c4fc17 <+0205> add %dh,%dl
0x00007fff88c4fc19 <+0207> add %al,(%rax)
0x00007fff88c4fc1b <+0209> add %dh,%dl
0x00007fff88c4fc1d <+0211> add %al,(%rax)
0x00007fff88c4fc1f <+0213> add %dh,%dl
0x00007fff88c4fc21 <+0215> add %al,(%rax)
0x00007fff88c4fc23 <+0217> add %ah,%ch
0x00007fff88c4fc25 <+0219> add %al,(%rax)
0x00007fff88c4fc27 <+0221> add %ah,%ch
0x00007fff88c4fc29 <+0223> add %al,(%rax)
0x00007fff88c4fc2b <+0225> add %ah,%ch
0x00007fff88c4fc2d <+0227> add %al,(%rax)
0x00007fff88c4fc2f <+0229> add %ah,%ch
0x00007fff88c4fc31 <+0231> add %al,(%rax)
0x00007fff88c4fc33 <+0233> add %ah,%ch
0x00007fff88c4fc35 <+0235> add %al,(%rax)
0x00007fff88c4fc37 <+0237> add %dh,%dl
0x00007fff88c4fc39 <+0239> add %al,(%rax)
0x00007fff88c4fc3b <+0241> add %dh,%dl
0x00007fff88c4fc3d <+0243> add %al,(%rax)
0x00007fff88c4fc3f <+0245> add %dh,%dl
0x00007fff88c4fc41 <+0247> add %al,(%rax)
0x00007fff88c4fc43 <+0249> add %dh,%dl
0x00007fff88c4fc45 <+0251> add %al,(%rax)
0x00007fff88c4fc47 <+0253> add %dh,%dl
0x00007fff88c4fc49 <+0255> add %al,(%rax)
0x00007fff88c4fc4b <+0257> add %dh,%dl
0x00007fff88c4fc4d <+0259> add %al,(%rax)
0x00007fff88c4fc4f <+0261> add %dh,%dl
0x00007fff88c4fc51 <+0263> add %al,(%rax)
0x00007fff88c4fc53 <+0265> add %dh,%dl
0x00007fff88c4fc55 <+0267> add %al,(%rax)
0x00007fff88c4fc57 <+0269> add %dh,%dl
0x00007fff88c4fc59 <+0271> add %al,(%rax)
0x00007fff88c4fc5b <+0273> add %dh,%dl
0x00007fff88c4fc5d <+0275> add %al,(%rax)
0x00007fff88c4fc5f <+0277> add %dh,%dl
0x00007fff88c4fc61 <+0279> add %al,(%rax)
0x00007fff88c4fc63 <+0281> add %dh,%dl
0x00007fff88c4fc65 <+0283> add %al,(%rax)
0x00007fff88c4fc67 <+0285> add %dh,%dl
0x00007fff88c4fc69 <+0287> add %al,(%rax)
0x00007fff88c4fc6b <+0289> add %dh,%dl
0x00007fff88c4fc6d <+0291> add %al,(%rax)
0x00007fff88c4fc6f <+0293> add %dh,%dl
0x00007fff88c4fc71 <+0295> add %al,(%rax)
0x00007fff88c4fc73 <+0297> add %dh,%dl
0x00007fff88c4fc75 <+0299> add %al,(%rax)
0x00007fff88c4fc77 <+0301> add %dh,%dl
0x00007fff88c4fc79 <+0303> add %al,(%rax)
0x00007fff88c4fc7b <+0305> add %dh,%dl
0x00007fff88c4fc7d <+0307> add %al,(%rax)
0x00007fff88c4fc7f <+0309> add %ah,%ch
0x00007fff88c4fc81 <+0311> add %al,(%rax)
0x00007fff88c4fc83 <+0313> add %dh,%dl
0x00007fff88c4fc85 <+0315> add %al,(%rax)
0x00007fff88c4fc87 <+0317> add %dh,%dl
0x00007fff88c4fc89 <+0319> add %al,(%rax)
0x00007fff88c4fc8b <+0321> add %dh,%dl
0x00007fff88c4fc8d <+0323> add %al,(%rax)
0x00007fff88c4fc8f <+0325> add %dh,%dl
0x00007fff88c4fc91 <+0327> add %al,(%rax)
0x00007fff88c4fc93 <+0329> add %dh,%dl
0x00007fff88c4fc95 <+0331> add %al,(%rax)
0x00007fff88c4fc97 <+0333> add %dh,%dl
0x00007fff88c4fc99 <+0335> add %al,(%rax)
0x00007fff88c4fc9b <+0337> add %dh,%dl
0x00007fff88c4fc9d <+0339> add %al,(%rax)
0x00007fff88c4fc9f <+0341> add %dh,%dl
0x00007fff88c4fca1 <+0343> add %al,(%rax)
0x00007fff88c4fca3 <+0345> add %dh,%dl
0x00007fff88c4fca5 <+0347> add %al,(%rax)
0x00007fff88c4fca7 <+0349> add %dh,%dl
0x00007fff88c4fca9 <+0351> add %al,(%rax)
0x00007fff88c4fcab <+0353> add %al,%ah

lee1210
Oct 22, 2010, 02:02 PM
What about the input file? Have you NSLog'd line,x,y,z? Declare a char ** that you assign &line to so strsep can change it. I'll keep asking if you read,compiled, and ran my example from post #29.

-Lee

Miglu
Oct 22, 2010, 02:08 PM
The code in post 29 is not relevant as this problem occurs in strtof's call stack, not strsep's. I print every coordinate. They are printed up to line 32 in the input file, then EXC_BAD_ACCESS stops it. There is nothing special in line 33.

lee1210
Oct 22, 2010, 02:19 PM
The code in post 29 is not relevant as this problem occurs in strtof's call stack, not strsep's. I print every coordinate. They are printed up to line 32 in the input file, then EXC_BAD_ACCESS stops it. There is nothing special in line 33.

Post at least line 33 of the file. Try moving line 33 to line 1 and see if the behavior changes. Have you fixed the memory leaks and the way values are returned?

-Lee

Miglu
Oct 22, 2010, 02:26 PM
Removing the line did not work.
0.612400; 0.353600; 0.707100;,
Every line ends in ;,

I deleted the malloc assign of x, y and z, but I am not sure what to do about the line one, as I did not fully understand your explanation of it. I tried deleting (char *) malloc(50), like I did for x, y and z, but then no lines were printed.

lee1210
Oct 22, 2010, 02:34 PM
OK, I turned this into C i could compile. How many lines are in the file? Somewhere between 525-550? Are you closing and reopening the file every time you are calling your function? If not, are you rewinding the FILE * in some other manner.

As for the free... it should be free(line), not free(&line).

-Lee

Miglu
Oct 22, 2010, 02:45 PM
There are 600 lines. I tried closing and reopening it and got 9 more lines printed. I think that it is because there was more time.
Doing free(line) causes exactly the same result, SIGABRT and the same thing printed to the console, as when free(&line) is called.

chown33
Oct 22, 2010, 02:50 PM
The code in post 29 is not relevant as this problem occurs in strtof's call stack, not strsep's. I print every coordinate. They are printed up to line 32 in the input file, then EXC_BAD_ACCESS stops it. There is nothing special in line 33.

If you expect someone to debug this, the first requirement is to replicate the problem. The only way to do that accurately is to process the same data that causes a crash.

Just because you don't see anything special in line 33 (or any other line) doesn't mean there isn't something there. Frankly, you're not the most reliable describer of data. You posted earlier about using fscanf, but failed to mention that the actual separator between numbers contained whitespace. It was necessary for you to post your data before someone else figured it out.

If there is some reason for not posting the data, then explain that reason.


Also, post your most recent code. I see a serious error in the most recently posted code, and I want to see if it's still in your code after your recent changes. The error is here:
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
}

You are creating a stack-local array of floats, then returning a pointer to it in *a. This local array will not survive after the return. It's ironic, because the one place where malloc makes some sense, you didn't use it, and all the places that a local array makes sense, you used malloc.

What this error confirms to me is that you have an inadequate understanding of pointers and arrays. You don't understand the correspondence between array and pointer, and you don't understand a stack-local array's lifetime (in C references, it's called lifetime of storage class 'auto').

lee1210
Oct 22, 2010, 03:32 PM
There are 600 lines. I tried closing and reopening it and got 9 more lines printed. I think that it is because there was more time.
Doing free(line) causes exactly the same result, SIGABRT and the same thing printed to the console, as when free(&line) is called.

You are now closing and reopening it every time you call this function? If not, try:
rewind(fp);

At the top of the function.

-Lee

EDIT: This isn't the right way to solve a problem, just trying to point out what the problem is. Don't read N lines each time you enter the function, just read the next one. If you have to be able to "randomly" seek to a line based on i you might be in some trouble, but if you just need to read "the next one" reading one line at a time in this function will suffice.

Miglu
Oct 23, 2010, 05:42 AM
x,y,z don't need to have memory allocated at all. Are you positive you understand pointers? You are assigning a pointer to some new memory to those variables, then immediately assign to them, losing the original pointer. You could never free the memory at that point.

I think that mallocing is necessary for x, y and z in char *x, *y, *z;
x = strsep(&line, ";");
y = strsep(&line, ";");
z = strsep(&line, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)}; When I do free(x) I get SIGABRT. This is from http://en.wikipedia.org/wiki/Malloc: "Another problem is when free is passed an address that was not allocated by malloc, realloc or calloc. This can be caused when a pointer to a literal string or the name of a declared array is passed to free."

lee1210
Oct 23, 2010, 07:26 AM
I think that mallocing is necessary for x, y and z in char *x, *y, *z;
x = strsep(&line, ";");
y = strsep(&line, ";");
z = strsep(&line, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)}; When I do free(x) I get SIGABRT. This is from http://en.wikipedia.org/wiki/Malloc: "Another problem is when free is passed an address that was not allocated by malloc, realloc or calloc. This can be caused when a pointer to a literal string or the name of a declared array is passed to free."

You should not malloc or free these variables. If you ran my strsep example you'd see what tmp is set to each iteration, which is an address in the memory pointed to by the argument to strsep. If you malloc those variables as soon as you assign a new pointer value to them from strsep you lose the pointer from malloc so you can never free that memory. If you want to see this in your own code NSLog or printf with the %p specifier and display lime, then x,y,z after the strsep calls.

You could avoid malloc and free for line by declaring:
char line[50];
char *lineptr = line;

Then passing lineptr to strsep.

How you return your 3 floats is another problem. You cannot return a pointer to stack memory. I would have a large float coords[600][3] outside of this function, and just pass coords[x] (type float *) to the function. Then you can just assign the floats to [1],[2],[3] in the function. malloc would work, too, but you have been struggling with that so this would allow you to avoid it.

-Lee

Miglu
Oct 23, 2010, 07:49 AM
Could you explain when I should and should not use malloc and when I should and should not use free, as that is usually not explained explicitly in C memory tutorials.

lee1210
Oct 23, 2010, 09:13 AM
Could you explain when I should and should not use malloc and when I should and should not use free, as that is usually not explained explicitly in C memory tutorials.

Only use free on a pointer that was returned from malloc or friends (calloc, etc.). I recommended against using malloc in this case because you were having problems with it and it's not strictly necessary here. It can be difficult (I have known professional programmers and seen code written for-pay that do it wrong) which is a major reason for retain-release in Apple's code and the later addition of Garbage Collection. Note these methods only apply to Objective-C objects, so while you're working with primitives you don't get to use them.

You should use malloc when:
1) you need "a lot" of memory. How much that is is hard to pin down, but the stack has far less room to grow than the heap, so huge arrays or very large structs should be allocated from heap memory.
2) when you can't know how large of an array you'll need at compile-time. In this case most people guess a large number, they are eventually wrong, and a buffer-overflow occurs. This could lead to a crash or cause erroneous behavior. This is also a common source of security exploits.
3) When values need to live after the return of a function and there isn't a passed-in place to store them. After a function returns (its stack frame is popped from the stack) the automatic/stack-local variables' memory is no longer valid to use. If you return a pointer to it, the values stored there will change once a new stack frame is pushed onto the stack and its memory is manipulated in the new function.

There are probably others, but those are the main ones that spring to mind.

Note that manual memory management is hard, and programmers better than us have implemented other memory management schemes because they know that most of us are bad at it. It is definitely worth knowing, but it seems like right now you are just trying to get your feet wet. I don't know exactly because you didn't answer when I asked about what your experience level is and what the purpose of this project is (which would really help in terms of tailoring answers and assumptions about previous knowledge).

-Lee

Miglu
Oct 23, 2010, 09:59 AM
Why can I not just use char line[50]? Why do I have to use lineptr which has been initialized using char line[50];
char *lineptr = line;

lee1210
Oct 23, 2010, 10:24 AM
Because strsep modifies the value you pass a pointer to. If you pass a pointer to a char[] the address of the char[] can't be modified like the pointer value of a char *. line cannot be changed if it is statically allocated, while a char * can be modified. Using malloc in this case for line is problematic because while line is a char * once you change it with strsep you don't have the original pointer to free.

To demonstrate how error-prone this can be, I made a few errors in my example (banged it out quickly). I never tried to free the calloc'd value, and couldn't have if I wanted to because the pointer was changed. A different pointer to a char[] is a better solution.
-Lee

Miglu
Oct 23, 2010, 11:35 AM
This is the latest code. I can not see any memory leaks. - (void) getCoords: (int) i Array: (float **) a {
fp2 = fopen("...Coordinates.txt", "r");
char line[50];
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2);
}
char *x, *y, *z;
x = strsep(&strptr, ";");
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
} The amount of lines that are printed before I get EXC_BAD_ACCESS has varied throughout my debugging. It is now 16.

lee1210
Oct 23, 2010, 12:01 PM
I'm not at a computer (not that you provided a compilable example), but you are now leaking file handles. I don't know that that's why things are going bad, but it's wrong and will cause a crash. Did you try rewinding fp as I suggested earlier?

Also, the way you're returning coords is still wrong.

-Lee

subsonix
Oct 23, 2010, 12:04 PM
This is the latest code. I can not see any memory leaks. - (void) getCoords: (int) i Array: (float **) a {
fp2 = fopen("...Coordinates.txt", "r");
char line[50];
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2);
}
char *x, *y, *z;
x = strsep(&strptr, ";");
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
} The amount of lines that are printed before I get EXC_BAD_ACCESS has varied throughout my debugging. It is now 16.

Here coords is local to getCoords, so, when you are leaving scope it will disappear. Your supplied pointer a, is now pointing nowhere. You are not copying the content of coords to a, but it's address.

You could solve this by adding x, y and z directly to a, since you have the pointer available inside your function. Of course you also then need to make sure you have room for x, y and z in it.

Miglu
Oct 23, 2010, 12:08 PM
Here coords is local to getCoords, so, when you are leaving scope it will disappear. Then why am I able to print the floats in the array a after calling this method?

Miglu
Oct 23, 2010, 12:09 PM
Call stack:
#0 0x7fff88c4fbeb in __strtodg
#1 0x7fff88c4fac3 in strtof_l
#2 0x100001fbf in -[OpenGLView getCoords:Array:] at OpenGLView.m:192
#3 0x100001586 in -[OpenGLView initWithFrame:] at OpenGLView.m:64
#4 0x7fff803aa198 in -[NSCustomView nibInstantiate]
#5 0x7fff80302263 in -[NSIBObjectData instantiateObject:]
#6 0x7fff8030164e in -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:]
#7 0x7fff802ffcd9 in loadNib
#8 0x7fff802ff1e9 in +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZone:ownerBundle:]
#9 0x7fff802ff021 in +[NSBundle(NSNibLoading) loadNibNamed:owner:]
#10 0x7fff802fc5a3 in NSApplicationMain
#11 0x100001011 in main at main.m:13

Sydde
Oct 23, 2010, 12:59 PM
As an exercise, take all of the declarations in your code and arrange them at the top, before the code of the routine. Then place comments before and after the declarations section that say " these are temporary variables that will not exist after this routine exits". Then examine your code and cross-reference what you to to the declarations block.

It is clearly a matter of style, but I find it easier to keep track of things when the declarations (locals) Are grouped at the top of a routine.

subsonix
Oct 23, 2010, 01:02 PM
Then why am I able to print the floats in the array a after calling this method?

Luck? But I might be missing something here... BUT, your function does not work as it should, the amount of success varies and the error is EXC_BAD_ACCESS, correct?

Miglu
Oct 23, 2010, 01:13 PM
Could you point out the problems: - (void)makeDisplayList {
fp = fopen("...CoordinateGroups.txt", "r");
fp2 = fopen("...Coordinates.txt", "r");

char *line = (char *) malloc(50);
line = fgets(line, 50, fp);
char *lineNumber = (char *)malloc(5);
char *geometricType = (char *) malloc(1);
int num;

glNewList(1, GL_COMPILE);
while (line) {
geometricType = strsep(&line, ";");
num = strtol(geometricType, NULL, 10);

//This is for GL_QUADS. I have not yet added GL_TRIANGLES coordinate parsing code.
if (num == 4) {
float *coords1 = (float*)malloc(3*sizeof(float));
float *coords2 = (float*)malloc(3*sizeof(float));
float *coords3 = (float*)malloc(3*sizeof(float));
float *coords4 = (float*)malloc(3*sizeof(float));
for (int i = 0; i < 4; i++) {
lineNumber = strsep(&line, ",");
if(strlen(lineNumber) > 0) {
if(lineNumber[strlen(lineNumber)-1] == ';') lineNumber[strlen(lineNumber)-1] = '\0';
}
if (i == 0) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords1];
printf("%f, %f, %f\n", coords1[0], coords1[1], coords1[2]);
} else if (i == 1) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords2];
printf("%f, %f, %f\n", coords2[0], coords2[1], coords2[2]);
} else if (i == 2) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords3];
printf("%f, %f, %f\n", coords3[0], coords3[1], coords3[2]);
} else {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords4];
printf("%f, %f, %f\n", coords4[0], coords4[1], coords4[2]);
}
}
glColor3f(1.0, 1.0, 1.0);
glBegin(GL_QUADS);
{
glVertex3f(coords1[0], coords1[1], coords1[2]);
glVertex3f(coords2[0], coords2[1], coords2[2]);
glVertex3f(coords3[0], coords3[1], coords3[2]);
glVertex3f(coords4[0], coords4[1], coords4[2]);
}
glEnd();
}
line = fgets(line, 50, fp);
}
glEndList();
free(line);
}

- (void) getCoords: (int) i Array: (float **) a {
rewind(fp2);
char line[50];
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2);
}
char *x, *y, *z;
x = strsep(&strptr, ";");
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
}
Call stack:
#0 0x7fff88c21d94 in __Balloc_D2A
#1 0x7fff88c2234b in __lshift_D2A
#2 0x7fff88c5103c in __strtodg
#3 0x7fff88c4fac3 in strtof_l
#4 0x100001fb9 in -[OpenGLView getCoords:Array:] at OpenGLView.m:196
#5 0x1000015f5 in -[OpenGLView makeDisplayList] at OpenGLView.m:74
#6 0x10000139b in -[OpenGLView initWithFrame:] at OpenGLView.m:39
#7 0x7fff803aa198 in -[NSCustomView nibInstantiate]
#8 0x7fff80302263 in -[NSIBObjectData instantiateObject:]
#9 0x7fff8030164e in -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:]
#10 0x7fff802ffcd9 in loadNib
#11 0x7fff802ff1e9 in +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZone:ownerBundle:]
#12 0x7fff802ff021 in +[NSBundle(NSNibLoading) loadNibNamed:owner:]
#13 0x7fff802fc5a3 in NSApplicationMain
#14 0x100001049 in main at main.m:13

lee1210
Oct 23, 2010, 01:21 PM
I said to use rewind.

There is a high probability that you are reading off the end of your file, so x y and z are null, so atof fails. That was happening before.

Most likely you're going to get the same pointer returned from every call. So you'll have the last coords in all of your coordsX variables. It will work until another function is called that changes this memory. Do you think we're lying about returning pointers to stack-local memory being wrong?

-Lee

Miglu
Oct 23, 2010, 01:32 PM
Look at getCoords, I use rewind.
The while loop ends when the end of the file is reached, so that is not the problem.

jared_kipe
Oct 23, 2010, 01:41 PM
This thread is amazing. Reminds me of some farmerdoug threads.

chown33
Oct 23, 2010, 03:25 PM
Could you point out the problems: ...



- (void)makeDisplayList {
fp = fopen("...CoordinateGroups.txt", "r"); // opened but never closed
fp2 = fopen("...Coordinates.txt", "r"); // opened but never closed

char *line = (char *) malloc(50); // will be null when free()'ed; reassigned
line = fgets(line, 50, fp); // may assign null; 50 is magic number
char *lineNumber = (char *)malloc(5); // never freed; reassigned
char *geometricType = (char *) malloc(1); // never freed; reassigned; inadequate length
int num; // not used outside while block, should be declared inside that block

glNewList(1, GL_COMPILE);
while (line) {
geometricType = strsep(&line, ";"); // reassigns to malloc'ed ptr
num = strtol(geometricType, NULL, 10); // long assigned to int: may lose precision

//This is for GL_QUADS. I have not yet added GL_TRIANGLES coordinate parsing code.
if (num == 4) {
float *coords1 = (float*)malloc(3*sizeof(float)); // never freed; reassigned
float *coords2 = (float*)malloc(3*sizeof(float)); // never freed; reassigned
float *coords3 = (float*)malloc(3*sizeof(float)); // never freed; reassigned
float *coords4 = (float*)malloc(3*sizeof(float)); // never freed; reassigned
for (int i = 0; i < 4; i++) {
lineNumber = strsep(&line, ",");
if(strlen(lineNumber) > 0) {
if(lineNumber[strlen(lineNumber)-1] == ';')
lineNumber[strlen(lineNumber)-1] = '\0';
} // no else clause, possible bug: what happens when strlen is 0?

if (i == 0) { // use switch/case instead of if/else chain
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords1];
printf("%f, %f, %f\n", coords1[0], coords1[1], coords1[2]);
} else if (i == 1) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords2];
printf("%f, %f, %f\n", coords2[0], coords2[1], coords2[2]);
} else if (i == 2) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords3];
printf("%f, %f, %f\n", coords3[0], coords3[1], coords3[2]);
} else {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords4];
printf("%f, %f, %f\n", coords4[0], coords4[1], coords4[2]);
}
// In all the above blocks, no error-check is done on strtol(),
// which returns 0 with errno=EINVAL on errors.
// Every call to getCoords:Array: overwrites a malloc'ed ptr
// with an out-of-scope ptr to a stack-local float[].
}
glColor3f(1.0, 1.0, 1.0);
glBegin(GL_QUADS);
{
glVertex3f(coords1[0], coords1[1], coords1[2]);
glVertex3f(coords2[0], coords2[1], coords2[2]);
glVertex3f(coords3[0], coords3[1], coords3[2]);
glVertex3f(coords4[0], coords4[1], coords4[2]);
}
glEnd();
}
line = fgets(line, 50, fp);
// This fgets() uses a value of line that was advanced by prior strsep() calls.
// The sequential actions of strsep() will eventually advance line past the end
// of the malloc'ed buffer, and subsequent fgets()'s will damage memory.
}
glEndList();
free(line); // will always free null, since line is null after EOF from fgets()
}

- (void) getCoords: (int) i Array: (float **) a {
rewind(fp2); // poor design choice: uses fp2 ivar instead of arg
char line[50]; // magic number
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2); // doesn't break or return on null (EOF)
}
char *x, *y, *z;
x = strsep(&strptr, ";");// 3X: doesn't handle null strptr
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};// 3X: doesn't handle conversion failures
*a = coords; // overwrites malloc'ed ptr; stores address of out-of-scope buffer
}


This code demonstrates a nearly complete lack of understanding of how malloc() and free() work. The design also betrays a lack of understanding of the correlation between a declared C array and a pointer to the array's type. In other words, if you understood the correlation between pointers and arrays, then a completely different and much simpler design would have resulted, and malloc would not even be needed, so the whole issue of using malloc properly would simply never occur.

This code contains so many serious and fundamental errors it would be simpler to rewrite it entirely, without the use of malloc(). This is not only possible, it is entirely practical, since none of the buffers need a dynamic length, nor are any of them large enough that stack space would be exhausted. Furthermore, redesigned code could simply use a 2D float array and eliminate any if/else or switch/case.

If you want the possibility of someone else helping to rewrite this code, you must post an example of the data being read from fp and from fp2. It's too difficult to decipher the data format from the code, since there are no comments or examples in the code itself. You should post enough example data for both files that someone else can actually write the new functions, and run them on the example data placed in test files.

If you are unwilling to post example data, it may take more time for someone to help you, because anyone wishing to do so will first have to make their own test data and run it through the existing code.

Miglu
Oct 23, 2010, 03:47 PM
Thanks. I am not advanced enough to rewrite it on my own, so I indeed need help. This is from CoordinateGroups: 4; 0, 3, 2, 1;,
4; 4, 7, 6, 5;,
4; 8, 11, 10, 9;,
4; 12, 15, 14, 13;, This is from Coordinates: 0.258800; 0.000000; -0.965900;,
0.224100; 0.129400; -0.965900;,
0.433000; 0.250000; -0.866000;,
0.500000; 0.000000; -0.866000;,
0.500000; 0.000000; -0.866000;,
0.433000; 0.250000; -0.866000;,
0.612400; 0.353600; -0.707100;,
0.707100; 0.000000; -0.707100;,
0.707100; 0.000000; -0.707100;,
0.612400; 0.353600; -0.707100;,
0.750000; 0.433000; -0.500000;,
0.866000; 0.000000; -0.500000;,
0.866000; 0.000000; -0.500000;,
0.750000; 0.433000; -0.500000;,
0.836500; 0.483000; -0.258800;,

lee1210
Oct 23, 2010, 09:51 PM
If that is your whole Coordinates file, then you're missing 1 line (line 15 with the first line being numbered 0) that the 4th line of CoordinateGroups wants to use.

I moved this to C because the code you posted wasn't full/compilable. I did my best to cobble together the OpenGL init pieces because they weren't included. I don't think the way I did it would cause issues. Here is the code:


#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <GLUT/glut.h>

FILE *fp,*fp2;
void getCoords(int i, float *a);

int main(int argc, char *argv[]) {
char *lineNumber,*geometricType,*lineptr;
char line[50];
int num;

fp = fopen("CoordinateGroups.txt", "r");
if(!fp) {
fprintf(stderr,"Could not open CoordinateGroups.txt");
return -1;
}

fp2 = fopen("floats.txt", "r");
if(!fp2) {
fprintf(stderr,"Could not open floats.txt");
return -2;
}


glutInit(&argc,argv);
glutInitDisplayMode(GLUT_SINGLE | GLUT_RGB | GLUT_DEPTH );
glutCreateWindow("Test");

glNewList(1, GL_COMPILE);
while (fgets(line,50,fp) != NULL) {
lineptr = line;
geometricType = strsep(&lineptr, ";");
if(!geometricType) {
fprintf(stderr,"Could not read the geometric type from line: %s\n",line);
return -3;
}
if(strlen(geometricType) == 0) {
fprintf(stderr,"Could not read the geometric type from line: %s\n",line);
return -4;
}

num = atoi(geometricType);

//This is for GL_QUADS. I have not yet added GL_TRIANGLES coordinate parsing code.
if (num == 4) {
float coords[4][3];
int i;
for (i = 0; i < 4; i++) {
lineNumber = strsep(&lineptr, ",");
if(!lineNumber) {
fprintf(stderr,"Invalid line specification read from line: %s\n",line);
return -5;
}
if(strlen(lineNumber) > 0) {
int len = strlen(lineNumber);
if(lineNumber[len-1] == ';') lineNumber[len-1] = '\0'; //Could probably be unconditional if you're positive about the format
} else {
fprintf(stderr,"Invalid line specification read from line: %s\n",line);
return -6;
}
getCoords(atoi(lineNumber),coords[i]);
printf("%f, %f, %f\n", coords[i][0], coords[i][1], coords[i][2]);
}
glColor3f(1.0, 1.0, 1.0);
glBegin(GL_QUADS);
{
glVertex3f(coords[0][0], coords[0][1], coords[0][2]);
glVertex3f(coords[1][0], coords[1][1], coords[1][2]);
glVertex3f(coords[2][0], coords[2][1], coords[2][2]);
glVertex3f(coords[3][0], coords[3][1], coords[3][2]);
}
glEnd();
}
}
glEndList();
fclose(fp);
fclose(fp2);
return 0;
}

void getCoords(int i, float *a) {
rewind(fp2);
char line[50];
char *strptr = line;
int j;
for (j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2);
if(!strptr) {
fprintf(stderr,"Could not read line %d from float.txt, trying to read to line %d.\n",j,i);
return;
}
}
for(j = 0; j < 3; j++) {
char *val = strsep(&strptr, ";");
if(!val) {
fprintf(stderr,"Invalid token in coordinates file, line: %s\n",line);
return;
}
a[j] = strtof(val,NULL);
}
}


I took out any dynamic memory allocation. If you have to maintain this code in the future, that seems for the best, and it wasn't necessary anyway. I added some error checking (this would have helped point out a lot of your problems, such as reading off the end of a file). A big fix is how getCoords returns the floats read that we'd been insisting you deal with. While the return values were correct right after you called the function, you never printed after subsequent calls, or calls to other functions. If you had, you'd have seen that your values were changing out from under your nose (I did this with the code you posted and saw this result). Now a pointer to one dimension of a statically-allocated 2D array of floats is passed in, and the values of the three floats read are stored immediately into this array. Another big fix is how line and lineptr are used so you don't overflow line, which seemed to be a big source of trouble. CoordinateGroups' first 3 lines are 47 characters, so once you read line 4 you were off the end of line and ruining other parts of the stack.

I didn't take care of:
Checking the error for atoi/strtof as chown33 suggested
Using the "magic" constant of 50 instead of a #define
Passing fp2 to getCoords (i just made fp and fp2 global because they weren't declared in the code you posted and i was just trying to get the code to compile)

I don't remember what else I changed and a diff would be hard to decipher because of the Objective-C to C changes, etc. but I'm sure you can make the comparisons. I think what you need to copy where should be apparent. All of the returns might need to be pulled out of main when you paste it back to makeDisplayList, but you should have some way to indicate an error has occurred.

I hope very much that this is a personal project for fun or to learn, and not a project for school or work. If it is for school hopefully a prof catches you cheating, if it's for work I hope you admit that you're in over your head if you're tasked with something like this in the future (also, providing an address where a consulting invoice could be mailed would be helpful if this is for work). I don't want to make accusations, but you refused to disclose what this is for after many requests.

In the future, providing all of your code and all of your data to begin with will save everyone a lot of second-guessing. Also, it might help to be more agreeable when people ask you if you've tried things or request details about a project or your skill level. Many times you insisted you understood pointers, but if so this is superficial and doesn't include an understanding of memory layout, memory management, etc. which is a critical part. This was at the core of many of the issues, and stating that your understanding was not particularly solid would have helped us guide you (likely guide you immediately away from dynamic memory management). When suggestions were made you were defensive or rude when everyone is trying to help you. No one is responding just to give you a hard time (not normally, at least). You might not get 100% correct information (I made an error in an example, and because i was looking at the thread on my phone missed that you had switched to rewind), but people are doing their best to assist you.

In any event, I hope this is helpful and that you've learned some things from myself and others providing advice in this thread.

-Lee

EDIT: It should be clear that i have no idea if this is error or bug free. I'm sure others can find problems with it. My goal was to make it better, but I'm sure it's not perfect (even beyond the shortcomings i detailed above).

chown33
Oct 24, 2010, 01:32 AM
Lee1210, thanks for saving me some typing. Your code is very nearly what I was thinking. As you did, I also intended to do it entirely in C, partly to show the design issues have nothing to do with Objective-C, and partly to show that studying and knowing plain ordinary C is not only worthwhile, but often mandatory.

One significant change is I was going to return either a boolean or a count from getCoords(), to indicate to the caller either success/fail or the number of correctly converted floats. The caller can then terminate the loop.

I was also considering using sscanf() in getCoords() to perform the conversions, partly because sscanf() will return a count of the number of items parsed. I think this format string (or something close to it) would have worked:
"%f; %f; %f;,"

A sscanf() conversion might also be possible when reading fp, but it might get complicated because of the different possibilities in number of items.

Finally, I wanted to add a brief comment about possible performance problems. There appears to be a regular pattern in the lines read, but this is not exploited at all. Instead, a file is read and re-read over and over. If the file is buffered or cached, or relatively short, this may not hamper performance. However, the algorithm appears to be on the order of N-squared, meaning that it will perform on the order of N-squared line-reads as the total number of coordinates (N) increases. This could become significant very quickly, and once performance begins to degrade, it will also get worse very quickly. There may be ways to improve this, but without knowing anything about the expected data or performance, it's impossible to guess what a successful improvement might be.

Miglu
Oct 24, 2010, 11:14 AM
Thanks. This was not a school or work project. I learned a lot about C from this.

lee1210
Oct 24, 2010, 12:31 PM
Hooray! 5 days, 78 posts, and over 1000 views, no ethical violations, and someone (maybe a few others that read it later could learn something, too) learned something. Overall a successful, if a bit drawn out, thread.

-Lee