Sign in to follow this  
siliconsurfer

Pointers and Char

Recommended Posts

Hi, I have problems with pointers and char's and can't just get my head around what's wrong! I have a function in C++ which reads in data from a file, I then want to compare the data read in, with a char* which is passed into the function. //The function loooks like this getCat(char *filename, char* inName); I declare a variable char *name; Then I read in the data using fgets(buffer, 255, fp); and read the data into the variable by doing sscanf(buffer, "%s", &name); Now I can print name to prove that the data is in there using printf("name = %s\n\n", &name); now I want to compare name with inName so I try if(strcmp(inName, name) == 0){ //do stuff } The problem is it crashes with a segmentation error. Any ideas what I'm doing wrong??

Share this post


Link to post
Share on other sites
Quote:
Original post by siliconsurfer

printf("name = %s\n\n", &name);


One problem is that ampersand before name in the printf. It should be:

printf("name = %s\n\n", name);

otherwise it's trying to print the pointer value itself as a string...

There may be other problems in your function, but that is impossible to tell without the full code...

Tom

Share this post


Link to post
Share on other sites
Well your main problem is that you've misspelt "string".:
void someNameBetterThanGetCatUnlessYouAreActuallyReturningAFeline(std::string filename, std::string expectedName)
{
std::ifstream reader(filename.c_str());
std::string name;
std::getline(reader, name);
if (name == expectedName)
{
// do stuff
}
}

Enigma

Share this post


Link to post
Share on other sites
Here is the complete code. The problem is when I try to do the strcmp on line 34. If I create a variable such as

char *newvar = "anystring";

And compare it with inUrl the code works fine. But comparing inUrl with domain causes the program to crash with a segmentation error. I presume this is to do with how sscanf works, but I can't figure out why, or how to fix it.



int getCat(char *filename, const char *inUrl)
18 {
19 FILE *fp;
20 char buffer[255];
21
22 char *domain;
23 //char *cat;
24
25 if((fp=fopen(filename, "r")) == NULL)
26 {
27 printf("Error opening file %s\n", filename);
28 return -1;
29 }
30 while(!feof(fp))
31 {
32 fgets(buffer, 255, fp);
33 sscanf(buffer, "%s", &domain);
34 if(strcmp(domain, inUrl) == 0){
35 printf("match found\n");
36 break;
37 }
38
39 }
40 fclose(fp);
41
42 return 0;
43 }


Share this post


Link to post
Share on other sites
you declare domain as a pointer to char. You need an array:


char domain[ MAX_SIZE ];
sscanf(buffer, "%s", domain);


Also, if you're doing this in C++, it's preferred to use std::string.

Share this post


Link to post
Share on other sites
if you have ANY trouble AT ALL using char* as a string type you should not be using it. The only time that you should use char* is when you are already quite experienced with std::string and have found one of those rare cases where using char* is better than using the real string type.


#include <string> //note, not string.h
#include <iostream>
#include <fstream>
using namespace std;

void GetFeline(string filename, string expected_data)
{
fstream fin(filename.c_str());
string actual_data;
fin>>actual_data;
if(actual_data==expected_data)
{
//blah blah blah
}
}

Share this post


Link to post
Share on other sites
Here's how I would do it.

#define STRING_LENGTH 255
int getCat(const char *filename, const char *inUrl)
18 {
19 FILE *fp;
20 char buffer[STRING_LENGTH];

if (filename == 0 || inUrl == 0)
{
return -1;
}

memset(buffer, 0, (sizeof(char) * STRING_LENGTH));
23 //char *cat;
24
25 if((fp=fopen(filename, "r")) == NULL)
26 {
27 printf("Error opening file %s\n", filename);
28 return -1;
29 }

30 while(!feof(fp))
31 {
32 fgets(buffer, STRING_LENGTH, fp);
34 if(strcmp(buffer, inUrl) == 0)
{
35 printf("match found\n");
36 break;
37 }
39 }
40 fclose(fp);
42 return 0;
43 }



[Edited by - the_dannobot on September 7, 2005 10:34:27 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by the_dannobot
Here's how I would do it.
#define STRING_LENGTH 255

Ewww. Yuck. It was better before you edited and fell back on the gutter of the preprocessor. This is C++. At the very least use a const integral type rather than tromping on everybodies names. Also both domain and the memset are totally redundant. And why a STRING_LENGTH of 255 rather than 256?

Seriously, it's much more readable, maintainable and extensible to use the SC++L and convert from and to char * string only when accesing C api calls:
// assumes getSiameseOrEgyptianMau is to be passed as a callback function
// to a C api, so uses char const * const parameters
int getSiameseOrEgyptianMau(char const * const filenameCharArray, char const * const inUrlCharArray)
{
std::string filename(filenameCharArray);
std::ifstream reader(filename.c_str());
if (!reader)
{
std::cout << "Error opening file " << filename << '\n';
return -1;
}
std::string inUrl(inUrlCharArray);
std::string domain;
while (reader >> domain)
{
if(domain == inUrl)
{
std::cout << "match found\n";
break;
}
}
return 0;
}



Enigma

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma
Quote:
Original post by the_dannobot
Here's how I would do it.
#define STRING_LENGTH 255

And why a STRING_LENGTH of 255 rather than 256?

I have no idea. That's what the OP was using.

Share this post


Link to post
Share on other sites
Quote:
Original post by Enigma

// assumes getSiameseOrEgyptianMau is to be passed as a callback function
// to a C api, so uses char const * const parameters
int getSiameseOrEgyptianMau(char const * const filenameCharArray, char const * const inUrlCharArray)
{
std::string filename(filenameCharArray);
std::ifstream reader(filename.c_str());
if (!reader)
{
std::cout << "Error opening file " << filename << '\n';
return -1;
}
std::string inUrl(inUrlCharArray);
std::string domain;
while (reader >> domain)
{
if(domain == inUrl)
{
std::cout << "match found\n";
break;
}
}
return 0;
}



Quoted for emphasis. This way of doing things:

- will work!
- easily interfaces with code that does use char*'s (shame on it)
- handles the memory management for you, in a way that won't limit your strings to any particular length (but also won't allocate a huge buffer if it isn't needed; there is a provable upper bound to the inefficiency)
- doesn't rely on hugely non-typesafe variadic functions (i.e. sscanf)
- is a bit shorter and definitely clearer
- is *definitely* more C++-idiomatic

The only thing I would change would be to construct the ifstream directly from the passed-in filename pointer; converting back and forth gains nothing here because we don't actually do any manipulation with that data. Oh, and inUrl doesn't need to be wrapped either since we just use it for comparison, and operator== is overloaded for (std::string, char*) appropriately. The real value here comes from using the local string variable, and the ability to insert into it directly from a new-style stream (FILE * is *ancient*! Why should we be thinking about this thing being a pointer rather than an object, anyway?)

Share this post


Link to post
Share on other sites
Zahlman makes a good point about the needless conversions of the parameters, however there is one more issue that we both missed, so allow me to present a (hopefully) final revision:
// assumes getAmericanBobtailOrPersian is to be passed as a callback function
// to a C api, so uses char const * const parameters
int getAmericanBobtailOrPersian(char const * const filenameCharArray, char const * const inUrlCharArray)
{
if (!filenameCharArray)
{
std::cout << "filename is null\n";
return -1;
}
if (!inUrlCharArrray)
{
std::cout << "url is null\n";
return -1;
}
std::ifstream reader(filenameCharArray);
if (!reader)
{
std::cout << "Error opening file " << filename << '\n';
return -1;
}
std::string domain;
while (reader >> domain)
{
if(domain == inUrlCharArray)
{
std::cout << "match found\n";
break;
}
}
return 0;
}

Enigma

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