C++ again...why does this throw an exception?

Recommended Posts

Edzio    122
string arg = string(argv[2]);//<-- if ( argc >= 3 && string(argv[1]) == "-f" ) { //string arg = string(argv[2]);//this goes to else ifstream infile; infile.open(argv[2]); if(infile.fail()) open_file_error(argv[2]); else random_map_generator(); } else { usage(string(argv[0])); }

Share on other sites
Edzio    122
string arg = string(argv[2]);
if ( argc >= 3 && string(argv[1]) == "-f" )
{
//string arg = string(argv[2]);
ifstream infile;
infile.open(argv[2]);
if(infile.fail()) open_file_error(arg);//<--oops!
else
random_map_generator();
}
else
{
usage(string(argv[0]));
}

Share on other sites
Edzio    122
that oops didnt fix it...i just copied the wrong code
so if it looks fixed because i reposted...hehe...
its not

Share on other sites
Edzio    122
that oops didnt fix it...i just copied the wrong code
so if it looks fixed because i reposted...hehe...
its not

Share on other sites
Edzio    122
sorry, that last post - the code is still wrong,
i just copied the wrong code...

Share on other sites
haegarr    7372
Hey, your code snippet seems me to be known by me ;)

Well, it would be better if you make the problem more clear. What exception is thrown? And under which conditions (i.e. commandline arguments) is it thrown? And what's the definition of open_file_error(...)?

What I see without all these informations is that you access argv[2] without having checked that the argv array is at least 3 elements big. Look at the if conditional I've given you earlier: It uses argc>3 to guarantee for the following code that the argv array is at least 3 elements big.

So, if you invoke your app like here
myapp
then argc will be 1 and argv will be argv[1] = {"myapp"}. Accessing argv[1] or above is hence invalid. Similarly, if you invoke it as
myapp -f
then argc will be 2 and argv = {"myapp", "-f"} and hence argv[2] isn't available yet! EVER test argc before accessing any argv[]; only argv[0] is guaranteed to be available!

Share on other sites
Aardvajk    13205
[EDIT - Bah, too slow [smile]]

Quote:
 Original post by Edziostring arg = string(argv[2]); if ( argc >= 3 && string(argv[1]) == "-f" ) { //string arg = string(argv[2]); ifstream infile; infile.open(argv[2]); if(infile.fail()) open_file_error(arg);//<--oops! else random_map_generator(); } else { usage(string(argv[0])); }

The main problem I can see is that you are assigning argv[2] to string arg before you are checking to see if there are >=3 arguments so if you only supply one or less args to the program when you run it, that would cause an access violation (not cause an exception to be thrown).

It looks as though your original was correct with the string arg=argv[2] line inside the first if bracket.

BTW, if you are assigning a const char * to a string or passing a const char * to a function expecting a string, you don't need to do string(x). That will be done automatically:

const char *c="hello";
string s=c; // fine

void f(const string &s){ }

f(c); // fine

Under what circumstances does it fail and what happens when it fails?

Share on other sites
Edzio    122
I dont know what I was doing but this works now / thanks everyone!

else if ( argc >= 3 && string(argv[1]) == "-f" )
{
string arg = string(argv[2]);
ifstream infile;
infile.open(argv[2]);
if(infile.fail()) open_file_error(arg);
else
random_map_generator();
}

Share on other sites
Aardvajk    13205
I don't mean to micro-optimise, but that snippet has a few redundant bits.

else if ( argc >= 3 && string(argv[1]) == "-f" ){ ifstream infile(argv[2]);if(infile.fail()) open_file_error(argv[2]);elserandom_map_generator(); }

You can open an ifstream in its constructor, which is better practise unless you have a good reason not to - you might forget what you are doing and place file reading code in between the line where you declare it and the line where you open it.

If a function is expecting a const std::string& or std::string, you can just pass a const char* argument to it and the temporary string will be created automatically, the advantage being that it will be destroyed at the end of the containing expression rather than the end of the block you declared your temporary "arg" in.

Share on other sites
Edzio    122
Thanks EasilyConfused,
I'll put that to good use.