John Matthews

+ Follow
since Jul 04, 2018
Was writing 6502 (8 bit) assembler fixed point arithmetic code (to draw circles) back in the early 80s, on an Acorn Atom if that means anything to anyone. Usually write embedded C, with the odd excursion into Java and C++.
Melksham, UK
Cows and Likes
Total received
In last 30 days
Total given
Total received
Received in last 30 days
Total given
Given in last 30 days
Forums and Threads
Scavenger Hunt
expand Rancher Scavenger Hunt
expand Ranch Hand Scavenger Hunt
expand Greenhorn Scavenger Hunt

Recent posts by John Matthews

Hanna Roberts wrote:

Suggestion: replace the 100 in the fgets() like this:
Then the code in the fgets() is independent of the size of the buffer, hence safer and more robust.
2 weeks ago
And fgets() also sets errno on error, so you can do something similar for that.
2 weeks ago
...or even better/simpler use the err() function:
2 weeks ago
Yes and yes

Although you could do a bit more. If you look at the documentation for fopen():

you'll see in 'return value' it says "Otherwise, NULL is returned and errno is set to indicate the error". You can make use of that with the strerror() function:

to give something like:
So for example if the file doesn't exist you might get this:
2 weeks ago
Hi Hanna - you just want one fgets(), so you could replace the while() with an if() (the if() would check that the line had been read ok, like the while() is doing).
2 weeks ago
Hi Mark

I don't think the error is caused by chars(), unless the input string is >19 chars in length (plus the \0 end-of-string character) in which case it will overflow s2[20] . However I think there's a logic bug - it will give the wrong result if the input string doesn't contain a character that only appears once eg. "abba".

The problem is in reverse(). I can see that you're trying to use recursion, and using s2[] to hold the reversed string. But every instance of reverse() which executes has its own s2[], and each s2[] only holds at most one character. And s2[] is a local variable, on the stack - when the function exits, s2[] 'disappears', so you can't return s2 from the function - that's probably what's causing the invalid memory reference when the returned value is used in the printf(). Similar problem with i - each reverse() has its own i.

You only want one s2[] and i - you could make them globals, defined outside the function, or it would be better to pass them into reverse() as arguments. In fact because you can't return arrays which are local variables from functions, it would probably be better to pass in the array to hold the reversed string as an argument. Something like:

3 weeks ago
I appreciate in your position you are looking at a lot more threads than I am. One is about my limit.
1 month ago

Tim Holloway wrote:What's more important: did you resolve your problem?

Don't get me started on my problems...
Oh, you meant the OP?
1 month ago

Tim Holloway wrote:Still, I do recommend manifest constants rather than "magic numbers", no matter what syntax you use.

Definitely. But would you define constant for the tab character '\t'? Or newline '\n'? Perhaps you would. But anyway, '\0' isn't any different to those - the number 0 isn't really a number in this context, it's just part of the 'symbol' for the NUL character.

And if you were going to use a constant for the end of string character, you wouldn't use NUL - that's not much better than 0. You would define something like EOS (which might be defined as NUL) to abstract the meaning from the value.
1 month ago

Tim Holloway wrote:You win . But as I said, in the real world, I've not seen it used.

I'm curious to know where that world is, because it's obviously very different to the un-real world I've been (professionally) writing C in, start-ups and multi-nationals, for the past 30-odd years where I don't remember anyone ever using anything other than '\0' as a string terminator. That's a genuine question. And again if you can show me anything online that uses/advises something different I'd be interested - C specifically as that's mostly what I do. You learn something new every day
1 month ago

Tim Holloway wrote:If, as I presume, you mean for '\0' to stand for the octal byte value \000, I'd recommend that you follow common practice and write all 3 digits.

I think the intention is to set the whole buffer to the end of string character '\0', which is at least logically different to '\000' and correct in this context. But as I mentioned above unnecessary.
1 month ago
Hi Adrian

Needed to fix a few things to get your code to compile. But the first real bug I spotted is:
strcat = string concatenation, and both arguments have to be strings. But c isn't a string - yes, it's a character, but to be a string length 1 it must be followed by the string terminator character '\0'. The way the code is at the moment it could be anything. This might result in a crash - either when the strcat() code goes off into invalid memory trying to find the '\0', or it finds one but the resulting string is too long to fit into left[] and it overwrites something it shouldn't.

Fix this, and it might work - I did, and I think it does.

A few points:
  • Why use (x - y == 0) instead of (x == y)?
  • To clear a string buffer you only need to set the first byte to '\0'; no need to memset() the whole buffer.
  • Even if this is only for your own use I would at least check the fopen() return value to verify that the file has been opened ok.
  • In your fgets() you use 100 to represent the size of buffer. But the size of buffer is sizeof(buffer) - much better to use that.
  • Not sure why you're using strtok() - you already have the string in buffer, what does strtok() with no delimiters give you?
  • You can use strchr() to check whether a character is one of a number of possibilities eg. strchr(":,->", c)

  • Cheers
    1 month ago