c Print out the letters, which were repeatedly in string

Question

My function should print out letters which are more than once in string. I have no idea why I get an empty output, or my program 'stops working'.

#include <string.h>
#include <stdio.h>

void funkcja3 (char []);

int main()
{
    funkcja3("napnapnaaw");
    return 0;
}

void funkcja3 (char napis[])
{
    int i=0,j;
    for(;i<strlen(napis);i++)
    {
        if((napis[i]>='a')&&(napis[i]<='z'))
        {
            int n=0;
            for(j=i+1;j<strlen(napis);j++)
            {
                if(napis[i]==napis[j])
                {
                    n++;
                    napis[j]=' ';
                }
            }
            if(n>0)
            {
                printf("%c ", napis[i]);
            }
        }
    }
}

`napid[j] = ' '` is trying to modify a literal string, which isn't allowed. — Barmar, Jan 27 '14 at 20:16
what @Barmar is saying is that *IF* you pass a non-modifiable string literal, you will have problems (UB). — Karoly Horvath, Jan 27 '14 at 20:20
@ViníciusGobboA.deOliveira How I can do that? I use Dev-cpp 5.5.3 — pw94, Jan 27 '14 at 20:21
It's not a 'correct behaviour' problem, but you should probably avoid calling `strlen(napis)` in the loop controls, especially the inner loop. Your algorithm is currently O(N^4) because of the two `strlen()` calls. — Jonathan Leffler, Jan 27 '14 at 20:22
@pw94 First of all, don't use Dev-C++. Second, read a good introductory book and learn the basics of C. — , Jan 27 '14 at 20:22
@KarolyHorvath I don't understand what do you mean. Could you point where the problem is? — pw94, Jan 27 '14 at 20:23
It seems to me that it's "International Why Writing A String Literal Crashes My Porgram" day today ([link](http://stackoverflow.com/questions/21387972/bad-permissions-for-mapped-region/21388029#comment32257941_21387972)) — , Jan 27 '14 at 20:24
@KarolyHorvath well, what kind of material would you suggest to someone who doesn't even know that string literals are read-only? (today this is the 3rd duplicate question.) — , Jan 27 '14 at 20:25
@H2CO3: intermediate? something like the equivalent of effective c++? — Karoly Horvath, Jan 27 '14 at 20:26
@KarolyHorvath Intermediate? Again, for someone who **doesn't even know that string literals are not modifiable?** Surely not. Also, this is a C question, suggesting a C++ book is entirely inappropriate. — , Jan 27 '14 at 20:27
@H2CO3: it's not as basic stuff as you would think. that's all I'm saying. — Karoly Horvath, Jan 27 '14 at 20:27
@KarolyHorvath Okay, maybe that's your opinion. Mine is different (it really is too basic. I remember that this was one of the very first things that I have learned about string literals.) — , Jan 27 '14 at 20:28

score 2 · Accepted Answer · answered Jan 27 '14 at 20:22

2

You need to pass a modifiable string:

int main()
{
    char str[] = "napnapnaaw";
    funkcja3(str);
    return 0;
}

answered Jan 27 '14 at 20:22

Barmar

741,623
53
500
612

score 2 · Answer 2 · answered Jan 27 '14 at 20:32

This is not a direct answer to your question, but if you just want to print out all the non-capital letters that appear more than once in a given string, then you can just use a histogram (symbol-counting) instead:

void funkcja3(char napis[])
{
    int histogram['z'-'a'+1] = {0};
    for (int i=0; napis[i]!=0; i++)
    {
        if ('a' <= napis[i] && napis[i] <= 'z')
            histogram[napis[i]-'a']++;
    }
    for (int i=0; i<'z'-'a'+1; i++)
    {
        if (histogram[i] > 1)
            printf("%c ",'a'+i);
    }
}

BTW, histogram = the number of occurrences of each symbol in the data.

score 0 · Answer 3 · answered Jan 27 '14 at 20:26

When I run your program on my machine (Ubuntu, gcc 4.6), I get a segmentation fault and a core dump. Giving the program and the core dump to gdb and doing a backtrace gives

$ gdb a.out core
Core was generated by `/tmp/a.out'.
Program terminated with signal 11, Segmentation fault.
#0  0x00000000004005a2 in funkcja3 (napis=0x40072c "napnapnaaw") at a.c:25
25                      napis[j]=' ';
(gdb) bt
bt
#0  0x00000000004005a2 in funkcja3 (napis=0x40072c "napnapnaaw") at a.c:25
#1  0x0000000000400520 in main () at a.c:8

This hint brings me to removing line 25 and running the program again

$ a.out
n a p n a a

which shows all characters, which were repeated somewhere later in the string.

score 0 · Answer 4 · answered Jan 27 '14 at 20:26

When you call funkcja3, you're calling it with a string literal. This string literal is at a memory location that isn't modifiable, so the call to napid[j] = ' ' should fail (and does so when I copy your example in to visual studio 2013). What you need to do is either A: use std::string (or another string implementation) or B: make a copy of the string in the function, examine it, and then delete the copy when done. Either way, you probably shouldn't be modifying the original string going in to the function. It's generally bad practice to modify objects passed in to a function unless the function absolutely has to do so.

There are some other ways of completing this task as well, such as an array of 26 shorts to hold the counts for each character. Make those counts and then print out any character that has more then 1.

Just realized this question was tagged specifically as C, which makes std::string not an option. The function should really copy the input string then so as to not modify it's input parameters. — Darinth, Jan 27 '14 at 20:30

c Print out the letters, which were repeatedly in string

4 Answers4