• Advertisement

to_lower function entering an infinite loop

Recommended Posts

I am super confused by this...

I was in the middle of an experiment, maybe is just that I am tired and I'm missing the obvious here (so facepalm is ready), but things are not working as expected, Image below (I'll also paste the code in case you want to try it out yourself)

#include <iostream>
#include <ostream>
using namespace std;

void to_lower(char* s)
{
	char* text = s;
	while (s != '\0')
	{
		char letter = *s;
		if (letter > 'A' && letter < 'Z')
		{
			*s = 'a' + (letter - 'A');
		}
		s++;
	}
	cout << text << endl;
}

int main()
{
	char* text = new char[12] {"Hello World"};
	to_lower(text);
	delete[] text;
	return 0;
}

As you see in the debug image, the content of s[0] is 'l', but "char letter = *s;" is yelding a 'e'...what is this madness?! O_O

 

Untitled-1.png

 

EDIT: forget what I asked above, I just understood that the yellow arrow means that the line isn't executed yet, I just had to step forward once :|

So, since that question turned useless I renamed this topic and I have a new question: my function enters an infinite loop.

Apparently the test "while (s != '\0')" is not good because when s points to '\0', the content of s is "" according to the debugger... so how do I test for '\0'? :P

 

Edited by MarcusAseth

Share this post


Link to post
Share on other sites
Advertisement

You're comparing a pointer, i.e. a memory address, against what you're expecting to see when dereferencing the pointer, i.e. the number 0 or '\0'. You need to dereference the pointer.

Also, your if statement is wrong.

And, you're wasting your time duplicating a standard language function.

Share this post


Link to post
Share on other sites
10 minutes ago, Kylotan said:

You're comparing a pointer, i.e. a memory address, against what you're expecting to see when dereferencing the pointer, i.e. the number 0 or '\0'. You need to dereference the pointer.

Also, your if statement is wrong.

And, you're wasting your time duplicating a standard language function.

ops, missed that dereference in the while, I knew I was tired x_x

Fixed my >= and <= , thanks! :D

I disagree on the last point though, I don't think this time is wasted, we are probably assigning value to different things :P

Edited by MarcusAseth

Share this post


Link to post
Share on other sites
9 minutes ago, Alpha_ProgDes said:

Though you wouldn't do this in production code, I agree you should know/understand how to handle pointers.

Exactly my goal here :P

What I'm currently writing is probably ugly and a bit buggy, but I'm getting useful practice, today first time I use const_cast and write my template, so...IMPROVEMENT (for better or worse...):D

char* findx(const char* s, const char* x)
{
	if(s == nullptr || x == nullptr)
	{ return nullptr; }

	char* Tmp = const_cast<char*>(x);
	int FindSize = 0;
	while (*Tmp++ != '\0') { FindSize++; };

	int ID = 0;
	int firstOccurrence = 0;
	bool isMatch = false;
	
	while (s[ID] != '\0')
	{
		if (s[ID] == x[0])
		{
			firstOccurrence = ID;
			for (size_t i = 0; i < FindSize; i++)
			{
				if (s[ID + i] != x[i]) { break; }
				if (i == FindSize - 1) { isMatch = true; }
			}
			if (isMatch) { break; }
		}
		ID++;
	}
	return (isMatch ? const_cast<char*>(s) + firstOccurrence : nullptr);
}

 

Edited by MarcusAseth

Share this post


Link to post
Share on other sites

Using const cast looks horribly wrong, "find" shouldn't change the string!

You have "char *tmp" and return a "char *",  change that to "const char *" too.

 

At a higher level, you are accepting const char * values as parameters, users don't expect that you break your promise, and sneakily return a modifiable point in their const string as return value.

Edited by Alberth

Share this post


Link to post
Share on other sites

The Exercise in the book I'm following gave as only instructions:

Quote

Write a function, char* findx(const char* s, const char* x), that finds the first occurrence of the C-style string x in s.

I don't know how to return a char* out of a const char*, so I tried removing the const, just to fullfill the assignment :P

Edited by MarcusAseth

Share this post


Link to post
Share on other sites

Here you go friend

//take two char* (strings) and return char* to first occurence of second string
char* findx(char* str, char* strx ) {
	
	if (!str || !strx) return nullptr;

	char* found = nullptr;

	for (int s = 0, x = 0; str[s] != '\0' && strx[x] != '\0'; s++) {

		if (str[s] == strx[x]) {
			if( !found ) found = &str[s];
			x++;
		} else {
			found = nullptr;
			x = 0;
		}
	}

	return found;
}

 

Share this post


Link to post
Share on other sites
11 hours ago, MarcusAseth said:

@h8CplusplusGuru much more clean/compact than mine, regardless if I had done it without the const char required by the exercise, thanks for showing me

 

 

Except that code doesn't find "abac" in the string "ababac". Correctness matters.

 

Share this post


Link to post
Share on other sites

lol xD 

Edit: since my lol above was downvoted, I realize it is kind of ambigous and possibly gives the wrong impression of what I laughed about, to put it in context: : it was because the other day I was saying to @h8CplusplusGuru that in my opinion the benefit of the forum was that anyone can correct each other and be corrected so that who's learning has more chance to get the correct informations. My lol was on the lines of "I knew it!"/"I said it"- don't hate pls :D

 

 

Edited by MarcusAseth

Share this post


Link to post
Share on other sites
On 9/14/2017 at 10:45 AM, alvaro said:

 

Except that code doesn't find "abac" in the string "ababac". Correctness matters.

 

I thought it was odd that there was only one loop!  Regardless, it was my intention to show that he's mostly poking around in the dark.

On 9/14/2017 at 10:59 AM, MarcusAseth said:

lol xD 

Edit: since my lol above was downvoted, I realize it is kind of ambigous and possibly gives the wrong impression of what I laughed about, to put it in context: : it was because the other day I was saying to @h8CplusplusGuru that in my opinion the benefit of the forum was that anyone can correct each other and be corrected so that who's learning has more chance to get the correct informations. My lol was on the lines of "I knew it!"/"I said it"- don't hate pls :D

I think the first line of what I had said was that forums such a gamedev are always around.

//take two char* (strings) and return char* to first occurence of second string
char* findx(char* str, char* strx ) {
	
	if (!str || !strx) return nullptr;


	for (int s = 0; str[s] != '\0'; s++) {

		if (str[s] != strx[0]) continue;

		char* found = &str[s];

		int x = 0;
		for ( x=1; strx[x] != '\0' && str[s + x] != '\0'; x++) {
			if (str[s + x] != strx[x]) 
				break;
		}
		if (strx[x] == '\0') return found;
	}

	return nullptr; 
}

Anyways, If you ever need a flashlight =)

Share this post


Link to post
Share on other sites
1 hour ago, h8CplusplusGuru said:

I thought it was odd that there was only one loop!  Regardless, it was my intention to show that he's mostly poking around in the dark.

Well it wasn't odd, you could do it with a single loop, I just did it out of curiosity to discover if I could do it. Seems I can :D
Let me know if you find any logic bug though, just to be sure this is viable.

char* findx(char* s, char* x)
{
	if (s == nullptr || x == nullptr) { return nullptr; }

	int Incremented = 0;
	for (; *s != '\0'; s++)
	{
       		if (*x == '\0') { break; }
		if (*s == *x)
		{
			x++;
			Incremented++;
			continue;
		}
		else
		{
			x -= Incremented;
			s -= Incremented;
			Incremented = 0;
		}
	}
	return (*x == '\0') ? s -= Incremented : nullptr;
}

 

Edited by MarcusAseth

Share this post


Link to post
Share on other sites

As a matter of style, I wouldn't check if the inputs are nullptr. Just like with standard-library functions, it's your responsibility not to pass null pointers around.

However, searching for the empty string should return the first location where that string can be found as a substring, which should be the first character.

Although the new code by h8CplusplusGuru is better, I think it doesn't do the right thing if you look for the empty string.

Share this post


Link to post
Share on other sites
35 minutes ago, alvaro said:

 I think it doesn't do the right thing if you look for the empty string.

I actually tested @h8CplusplusGuru  code with your previous parameters findx("ababac", "abac") and it return nulltpr, so something else is wrong beside passing an empty string :/

Share this post


Link to post
Share on other sites
1 hour ago, MarcusAseth said:

I actually tested @h8CplusplusGuru  code with your previous parameters findx("ababac", "abac") and it return nulltpr, so something else is wrong beside passing an empty string :/

Hmmm... I'm not sure how you tried, but it seems to work for me.

Share this post


Link to post
Share on other sites
2 minutes ago, alvaro said:

Hmmm... I'm not sure how you tried, but it seems to work for me.

My bad, I just realized I had scrolled past the latest example and compiled the first (old) example x_x

Edited by MarcusAseth

Share this post


Link to post
Share on other sites

I'm fully aware it can be done with a single loop, lol. My thought was that It didn't check previous values in the array, most easily solved with a second loop rather than contriving something with a single loop.

char* findx(char* str, char* strx)
{
	if (str == nullptr || strx == nullptr) { return nullptr; }

	char* found = nullptr;
	int s = 0, x = 0;

	while (str[s] != '\0') {

		if (!found) found = &str[s];
		if (strx[x] == '\0') return found;

		if (str[s] != strx[x]) {
			found = nullptr;
			s = s-x +1;
			x = 0;
		} else {
			s++;
			x++;
		}
	}

	return nullptr;
}

 

 

 

Edited by h8CplusplusGuru

Share this post


Link to post
Share on other sites

@h8CplusplusGuru  personally I like this version better for readability, I wonder if there is a way to time it (since it run probably too fast) to see if is better performant than the double loop version.

Only thing is it should return found instead of nullptr at the end :P

 

Edited by MarcusAseth

Share this post


Link to post
Share on other sites
39 minutes ago, h8CplusplusGuru said:

Why should it return found? If it gets to that line it's always a nullptr.

Try it with the findx("ababac", "abac"), it gets there and return a nullptr instead of the substring starting at found. That's why it should return found.

Edited by MarcusAseth

Share this post


Link to post
Share on other sites

This thread is a great example of why it is important to write tests.  We have at least five different implementations in this thread of `findx`, each with varying levels of “correct”.  Automated tests can provide an empirical measure of correctness.  And since `findx` is a pure function, we can easily write automated tests to help us.

First, a crude testing routine for any `findx` implementation:

const char* findx(const char*, const char*);

void test_findx() {
	struct test_t {
		const char* input;
		const char* subject;
		const char* expected;

		test_t(const char* input, const char* subject, const int pos) {
			this->input    = input;
			this->subject  = subject;
			this->expected = ( pos == -1 ) ? nullptr : (input + pos);
		}
	} tests[] = {
		test_t("abc",         "abc",  0),
		test_t("abcdef",      "abc",  0),
		test_t("ababac",      "abac", 2),
		test_t("ababac-----", "abac", 2),

		test_t("abc",         "xyz",  -1),
		test_t("abc",         "",     -1),
		test_t("",            "",     0),
		test_t("",            "a",    -1),

		// ...
	};

	int failed = 0;
	for ( const auto& test : tests ) {
		const char* result = findx(test.input, test.subject);

		if ( result != test.expected ) {
			failed ++;
			cerr << "Test #" << (&test - tests) << " failed" << endl;
		}
	}

	return failed;
}

And running the full example test here:  https://ideone.com/G2Suog#stdout

Test #0 failed: "abc" in "abc", expected "abc", got nullptr
Test #2 failed: "abac" in "ababac", expected "abac", got nullptr
Test #5 failed: "" in "abc", expected nullptr, got "abc"
Test #6 failed: "" in "", expected "", got nullptr
Failed 4 of 8 tests.

... we can see that there are a few cases we’ve missed.  And while we continually fix the implementation, we can think of more corner cases to add to the tests.

Testing can go along way in helping you write correct code and can also help you keep your sanity in face of bizarre bugs.

Share this post


Link to post
Share on other sites

mine passes the tests, yay! :D

well, actually it "fails" this 

Quote

test_t("abc",         "",     -1)

Test #5 failed: "" in "abc", expected nullptr, got "abc"

but I don't consider it a fail, because my original intention was indeed to return the left-hand side argument in case someone did something as pointless as to try to find nothing. But I don't actually know how the usual function decide to handle that case, so I will assume they return nullptr as expected by the testcase.

Anyway I've already saw in the code above something interesting I wasn't fully aware I could write, so I go back to read carefully trough it, thanks for posting it @fastcall22 :)

 

Edited by MarcusAseth

Share this post


Link to post
Share on other sites

As a mathematician, the desired behavior of findx when the second argument is the empty string is obvious: The first instance of the empty string is found at the beginning of the string.

For instance, I could make a function that splits a string into substrings using a separator, like "split" in Perl. This is how that Perl function behaves:

	$ perl -e 'for $x (split ",", "a,b,c") { print "\"$x\"\n";}'
	"a"
	"b"
	"c"
	$ perl -e 'for $x (split "", "a,b,c") { print "\"$x\"\n";}'
	"a"
	","
	"b"
	","
	"c"

So if you look for the empty string, you find it everywhere, and "split" separates every character.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now


  • Advertisement
  • Advertisement
  • Popular Tags

  • Advertisement
  • Popular Now

  • Similar Content

    • By mister345
      Hi guys, so I have about 200 files isolated in their own folder [physics code] in my Visual Studio project that I never touch. They might as well be a separate library, I just keep em as source files in case I need to look at them or step through them, but I will never actually edit them, so there's no need to ever build them.
      However, when I need to rebuild the entire solution because I changed the other files, all of these 200 files get rebuilt too and it takes a really long time.
      If I click on their properties -> exclude from build, then rebuild, it's no good because then all the previous built objects get deleted automatically, so the build will fail.
      So how do I make the built versions of the 200+ files in the physics directory stay where they are so I never have to rebuild them, but
      do a normal rebuild for everything else? Any easy answers to this? The simpler the better, as I am a noob at Visual Studio settings. Thanks.
    • By Snaked
      Im working in this project for 1 year .... mostly i develop a tool and databases for make the different maps and now i'm doing the client for play the game
      Tell me if you like it......
      this is a capture of how is viewing atm

       
       
      https://youtu.be/9251v4wDTQ0
    • By reenigne
      For those that don't know me. I am the individual who's two videos are listed here under setup for https://wiki.libsdl.org/Tutorials
      I also run grhmedia.com where I host the projects and code for the tutorials I have online.
      Recently, I received a notice from youtube they will be implementing their new policy in protecting video content as of which I won't be monetized till I meat there required number of viewers and views each month.

      Frankly, I'm pretty sick of youtube. I put up a video and someone else learns from it and puts up another video and because of the way youtube does their placement they end up with more views.
      Even guys that clearly post false information such as one individual who said GLEW 2.0 was broken because he didn't know how to compile it. He in short didn't know how to modify the script he used because he didn't understand make files and how the requirements of the compiler and library changes needed some different flags.

      At the end of the month when they implement this I will take down the content and host on my own server purely and it will be a paid system and or patreon. 

      I get my videos may be a bit dry, I generally figure people are there to learn how to do something and I rather not waste their time. 
      I used to also help people for free even those coming from the other videos. That won't be the case any more. I used to just take anyone emails and work with them my email is posted on the site.

      I don't expect to get the required number of subscribers in that time or increased views. Even if I did well it wouldn't take care of each reoccurring month.
      I figure this is simpler and I don't plan on putting some sort of exorbitant fee for a monthly subscription or the like.
      I was thinking on the lines of a few dollars 1,2, and 3 and the larger subscription gets you assistance with the content in the tutorials if needed that month.
      Maybe another fee if it is related but not directly in the content. 
      The fees would serve to cut down on the number of people who ask for help and maybe encourage some of the people to actually pay attention to what is said rather than do their own thing. That actually turns out to be 90% of the issues. I spent 6 hours helping one individual last week I must have asked him 20 times did you do exactly like I said in the video even pointed directly to the section. When he finally sent me a copy of the what he entered I knew then and there he had not. I circled it and I pointed out that wasn't what I said to do in the video. I didn't tell him what was wrong and how I knew that way he would go back and actually follow what it said to do. He then reported it worked. Yea, no kidding following directions works. But hey isn't alone and well its part of the learning process.

      So the point of this isn't to be a gripe session. I'm just looking for a bit of feed back. Do you think the fees are unreasonable?
      Should I keep the youtube channel and do just the fees with patreon or do you think locking the content to my site and require a subscription is an idea.

      I'm just looking at the fact it is unrealistic to think youtube/google will actually get stuff right or that youtube viewers will actually bother to start looking for more accurate videos. 
    • By mister345
      Hi, can someone please explain why this is giving an assertion EyePosition!=0 exception?
       
      _lightBufferVS->viewMatrix = DirectX::XMMatrixLookAtLH(XMLoadFloat3(&_lightBufferVS->position), XMLoadFloat3(&_lookAt), XMLoadFloat3(&up));
      It looks like DirectX doesnt want the 2nd parameter to be a zero vector in the assertion, but I passed in a zero vector with this exact same code in another program and it ran just fine. (Here is the version of the code that worked - note XMLoadFloat3(&m_lookAt) parameter value is (0,0,0) at runtime - I debugged it - but it throws no exceptions.
          m_viewMatrix = DirectX::XMMatrixLookAtLH(XMLoadFloat3(&m_position), XMLoadFloat3(&m_lookAt), XMLoadFloat3(&up)); Here is the repo for the broken code (See LightClass) https://github.com/mister51213/DirectX11Engine/blob/master/DirectX11Engine/LightClass.cpp
      and here is the repo with the alternative version of the code that is working with a value of (0,0,0) for the second parameter.
      https://github.com/mister51213/DX11Port_SoftShadows/blob/master/Engine/lightclass.cpp
    • By Rannion
      Hi, I am sending data to peers and those data need to be retreived from a scenegraph with a mutex to lock the data.
      The process of gathering the data is taking a bit less than a ms. I'm starting the thread every time I want to gather the data. If I'm running at 60 fps, I'm starting the thread 60 times per second so is that a performance or design problem?
      Would it be much better to have the thread always running and some kind of mechanism to ask him to perform the task whenever it's needed, so around 60 or 120fps?
      Also, does starting a thread creates some memory alloc/dealloc and then produce on the long run some kind of fragmentation?
      Thank you all.
  • Advertisement