Sign in to follow this  
Chris528

Problem with strings

Recommended Posts

Chris528    186
I'm getting a strange error with my code. Some code that has worked for the entire time I was working on my project(and has not been changed) has suddenly started to malfunction. Any ideas on what I might be doing wrong?
The problem is with the line app = replaceAll(app,"%basedir%",GetBaseDir()); %basedir % gets replaced with a garbage value. I'm positive that the GetBaseDir function is working as the messagebox function in GetBaseDir shows the correct value.
The first 3 functions are from a header file.
static char* GetBinDir()
{
	char pszModuleName[MAX_PATH];
	char* text = pszModuleName;
	GetModuleFileNameA( NULL, pszModuleName, MAX_PATH );
	char	drive[ MAX_PATH ];
	char	dir[MAX_PATH];
	_splitpath( pszModuleName, drive, dir, NULL, NULL );
	char binDir[MAX_PATH];
	_snprintf( binDir, sizeof(binDir), "%s%s", drive, dir );
	
	remove_trailing_slash( binDir );
	//MessageBoxA( NULL, binDir, "bin directory", MB_OK | MB_ICONINFORMATION | MB_APPLMODAL );

	return binDir;
}

static char* GetBaseDir()
{
	char baseDir[MAX_PATH];
	_snprintf(baseDir,sizeof(baseDir),"%s",GetBinDir());
	strip_last_dir( baseDir, MAX_PATH );
	remove_trailing_slash( baseDir );
	return baseDir;
}

static std::string& replaceAll(std::string& context, std::string const& from, std::string const& to)
{
    std::size_t lookHere = 0;
    std::size_t foundHere;
    while((foundHere = context.find(from, lookHere)) != std::string::npos)
    {
          context.replace(foundHere, from.size(), to);
          lookHere = foundHere + to.size();
    }
    return context;
}

void OnRunProgram(WebView* caller, const JSArray& args)
{
	WebString WS_APP = args[0].ToString();
	std::string app = ToString(WS_APP);

	app = replaceAll(app,"%gamedir%",configsHandle->m_Configs[configsHandle->currentConfig]->m_gamedir);
	app = replaceAll(app,"%basedir%",GetBaseDir());
		
	STARTUPINFOA si;
	memset( &si, 0, sizeof( si ) );
	si.cb = sizeof( si );

	PROCESS_INFORMATION pi;
	memset( &pi, 0, sizeof( pi ) );

	char binDirectory[MAX_PATH];
	_snprintf(binDirectory, sizeof(binDirectory), "%s", GetBinDir());
	DWORD dwFlags = 0;
	if ( !CreateProcessA( 
		0,
		(LPSTR)app.c_str(), 
		NULL,							// security
		NULL,
		TRUE,
		dwFlags,						// flags
		NULL,							// environment
		binDirectory,	// current directory
		&si,
		&pi ) )
	{
		MessageBoxA( NULL, app.c_str(), "Error", MB_OK | MB_ICONINFORMATION | MB_APPLMODAL );
	}
	else
	{
		//MessageBoxA( NULL, app.c_str(), "success", MB_OK | MB_ICONINFORMATION | MB_APPLMODAL );
	}
}

Edited by Chris528

Share this post


Link to post
Share on other sites

You are returning pointers to local variables which go out of scope after you return (return binDir in GetBinDir, return baseDir in GetBaseDir). The reason it may seem to work is that if you don't call any other functions after the called function returns the stack frame for the called function still has the values from the called function, i.e. you have invoked undefined behaviour and you have been lucky to get away with it.

 

If you turn the warning level up to the max your compiler will probably warn you about returning pointers to locals.

 

You are using C++ though, just return a std::string and don't bother with char*... if you need a char* to call an API function use std::string::c_str function.

 

The other alternative, if you really must use char*, is to get the caller to provide a buffer to be filled by the function. But really, Just use std::string instead...

Edited by Paradigm Shifter

Share this post


Link to post
Share on other sites
Chris528    186
Thank you very much for your help. I changed GetBinDir and GetBaseDir to use strings and it works perfectly now.
static std::string GetBinDir()
{
	char pszModuleName[MAX_PATH];
	char* text = pszModuleName;
	GetModuleFileNameA( NULL, pszModuleName, MAX_PATH );
	char	drive[ MAX_PATH ];
	char	dir[MAX_PATH];
	_splitpath( pszModuleName, drive, dir, NULL, NULL );
	char binDir[MAX_PATH];
	_snprintf( binDir, sizeof(binDir), "%s%s", drive, dir );
	
	remove_trailing_slash( binDir );
	//MessageBoxA( NULL, binDir, "bin directory", MB_OK | MB_ICONINFORMATION | MB_APPLMODAL );

	std::string returnval = binDir;
	return returnval;
}

static std::string GetBaseDir()
{
	char baseDir[MAX_PATH];
	_snprintf(baseDir,sizeof(baseDir),"%s",GetBinDir().c_str());
	strip_last_dir( baseDir, MAX_PATH );
	remove_trailing_slash( baseDir );

	std::string returnval = baseDir;
	return returnval;
}

Share this post


Link to post
Share on other sites
Ectara    3097

Thank you very much for your help. I changed GetBinDir and GetBaseDir to use strings and it works perfectly now.

static std::string GetBinDir()
{
	char pszModuleName[MAX_PATH];
	char* text = pszModuleName;
	GetModuleFileNameA( NULL, pszModuleName, MAX_PATH );
	char	drive[ MAX_PATH ];
	char	dir[MAX_PATH];
	_splitpath( pszModuleName, drive, dir, NULL, NULL );
	char binDir[MAX_PATH];
	_snprintf( binDir, sizeof(binDir), "%s%s", drive, dir );
	
	remove_trailing_slash( binDir );
	//MessageBoxA( NULL, binDir, "bin directory", MB_OK | MB_ICONINFORMATION | MB_APPLMODAL );

	std::string returnval = binDir;
	return returnval;
}

static std::string GetBaseDir()
{
	char baseDir[MAX_PATH];
	_snprintf(baseDir,sizeof(baseDir),"%s",GetBinDir().c_str());
	strip_last_dir( baseDir, MAX_PATH );
	remove_trailing_slash( baseDir );

	std::string returnval = baseDir;
	return returnval;
}

As an aside, that may not be too relevant, if you are using std::strings, it'd be best to stick to using it everywhere in the function that you can, rather than manipulating a C char array, then making a full copy of it (can be slow) just to return it. std::string's have an added benefit of caching the length, so things like removing trailing slashes can be made a lot simpler by checking the last character and optionally removing the last character if it is a directory separator, rather than checking every character in the array until you find the end (!), then determining if you should remove the last character. This can be especially wasteful if strip_last_dir() seeks until the null-terminator as well, finding the string's length twice in a row.

 

Additionally, strip_last_dir can easily be implemented using the std::string::rfind() to find the last directory separator (or second to last, if the last character is a separator, allowing you to handle both at once), and erasing every character from the found separator onward.

 

GetBaseDir() could look something like this:

static std::string GetBaseDir()
{
    std::string dir = GetBinDir();
    std::string::size_type index = dir.rfind("\\/:");
    
    if(index == dir.size() - 1)
    {
        //Trailing separator. Go back to the previous one.
        index = dir.rfind("\\/:", index);
    }
    
    if(index == std::string::npos)
    {
        //No separator indicating a parent directory!
        //Maybe return "." or something?
    }else{
        //Strip last directory, including a trailing separator.
        dir.erase(index);
    }
    
    return dir;
}

Obviously, this solution doesn't take locales into consideration, may have a bug or two, and does not consider too many edge cases. But, this is how one might choose to write it in C++98.

TL;DR:
I amend


don't bother with char*... if you need a char* to call an API function use std::string::c_str function.

to "don't bother with character arrays in general, until you are at the moment where you need to interface with an API function, then use std::string::c_str() or construct a string from a temporary buffer filled from an API function call". Sticking a std::string here and there for convenience, while still using character arrays, creates a bigger performance and design issue.

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

Sign in to follow this