Skip to content

Logic Error fixed in Solution #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 15, 2024
Merged

Logic Error fixed in Solution #72

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85067db
Logic Error fixed
SamiNasry Aug 28, 2024
3fb4576
Update loop.c to be more compact
SamiNasry Sep 12, 2024
44d3f7c
Update loop.c to be more easy to read.
SamiNasry Sep 12, 2024
12eb91d
Update loop.c
ohkimur Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions chapter_2/exercise_2_02/loop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,17 @@ int main(void)
if (i >= (MAXLINE - 1))
{
loop = 0;
break;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamiNasry Can we have it without the break until we figure out what's up with that? I find it very educational to learn how to deal with the differences between different machines.

PS: I'm used to writing code in high-level programming languages, and I almost forgot about all these low-level implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I removed the break until we figure out why it acts like this. Thanks for your time.

}
else if (c == '\n')
{
loop = 0;
break;
}
else if (c == EOF)
{
loop = 0;
break;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamiNasry This solution works well. We can improve the code by merging all the conditions into one condition with logical OR. Then, we can have only one set for loop = 0 and only one break.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, let's convert this:

if (i >= (MAXLINE - 1))
{
  loop = 0;
  break;
}
else if (c == '\n')
{
  loop = 0;
  break;
}
else if (c == EOF)
{
  loop = 0;
  break;
}

into something like this:

if (i >= (MAXLINE - 1) || c == '\n' || c == EOF)
{
  loop = 0;
}

I find this approach more compact and a bit easier to read. If you have a different opinion, please let me know.

@illustrofia mentions this is also an approach worth considering. I like it, but we're doing the same operation many times, which is a bit counterproductive. Also, having the same thing happening multiple times in multiple places will facilitate a pattern that might end up in divergence over time, especially in an environment where many devs are involved.

if (i >= (MAXLINE - 1))
{
  loop = 0;
}

if (c == '\n')
{
  loop = 0;
}

if (c == EOF)
{
  loop = 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ohkimur The more compact solution is good and easier to read but it doesn't fix the problem, you need to add the break after loop = 0, to get out of the while loop. So you change it into this :

if (i >= (MAXLINE - 1) || c == '\n' || c == EOF)
{
  loop = 0;
  break;
}

This fix is good and easier to read.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamiNasry I agree with you, except for the break, it doesn't make logical sense to me. I'll investigate it a bit and come back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, try to get the original getline function and move the LIMIT to 10 and in your code move the MAXLINE to 10 and try to type strings with 10 charchters or more you will find the behavior is not the same for both codes. Without the break the code adds an extra charachter.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamiNasry I think I identified the potential issue. We're using different platforms. I'm using macOS, which is UNIX-like, and you're using Windows. For some reason, the Windows build behaves differently.

Here is the behavior on my machine:
image

If I put the break, it basically doesn't insert the new line char:
image

So, I basically propose to use this approach:

if (i >= (MAXLINE - 1) || c == '\n' || c == EOF)
{
  loop = 0;
}```

If it still doesn't print the last char on windows, please investigate why that might happen and let me know. I'm very curious what would be the best approach to handle these differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right we have different behaviors. That's an interesting problem I will look into it further.
This is how it acts for me with the break
image

This is how it acts without the break
image

Anyways I will try to look into why it acts like this in different machines. For now let's just use the more compact solution. Thanks for your time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please show me how does the original getline function behave for the same input in your machine ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the code of the original getline() funtion

#include <stdio.h>

int getline(char s[], int lim);

int getline(char s[], int lim)
{
	int c,i;
	for (i = 0; i < lim -1 && ((c = getchar()) != EOF) && c != '\n' ; ++i)
	{
		s[i] = c;

	}
	if (c == '\n') {
		s[i] = c;
		++i;
	}
	s[i] = '\0';
	return i;
}


int main()
{
	int lim = 10;
	char s[lim];

	
	getline(s, lim);
	printf("%s" , s);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@SamiNasry Here is how it behaves for me. I had to change the name, since the clang compiler doesn't like me to reuse the getline name since it's already part of the stdio lib.


s[i++] = c;
Expand Down
Loading