LinuxQuestions.org
Help answer threads with 0 replies.
Go Back   LinuxQuestions.org > Forums > Non-*NIX Forums > Programming
User Name
Password
Programming This forum is for all programming questions.
The question does not have to be directly related to Linux and any language is fair game.

Notices


Reply
  Search this Thread
Old 12-18-2017, 11:09 PM   #1
Andy Alt
Member
 
Registered: Jun 2004
Location: Minnesota, USA
Distribution: Slackware64-stable, Debian64 stable, LFS 7.1
Posts: 489

Rep: Reputation: 161Reputation: 161
[C] function to replace part of a string - request for code review


The is a function I wrote. Does anyone have suggestions for improvements that can be made? Thanks.

Code:
/*
 * strreplc.c: replace a string inside of a string
 *
 * Copyright 2017 Andy <andy@oceanus>
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
 * MA 02110-1301, USA.
 *
 *
 */

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

char*
strreplc (const char *orig, const char *to_replace, const char *replace_with);

int main(int argc, char **argv)
{
  /* All the work is done in the function strreplc(). The code in main serves
   * as an example */
  char *string1 = "This program is distributed in the hope that it will be useful";
  char *replacement = "_inserted_string_";
  char *text_to_replace = "that it";

  char *output = strreplc (string1, text_to_replace, replacement);

  if (output != NULL)
  {
    printf ("%s\n", output);
  }

  free (output);
  return 0;
}

char*
strreplc (const char *orig, const char *to_replace, const char *replace_with)
{
  char *str = calloc (strlen (orig) + 1, 1);
  strcpy (str, orig);
  /* store the address of str */
  char *addr_str = &str[0];

  str = strstr (str, to_replace);

  if (str == NULL)
  {
    printf ("string to replace not found\n");
    return NULL;
  }

  int len_start = strlen (addr_str) - strlen (str);

  int len_middle = strlen (replace_with);

  int len_end = strlen (str) - strlen (to_replace);

  char *string2 = calloc (len_start + len_middle + len_end + 1, 1);

  strncpy (string2, addr_str, len_start);
  strcat (string2, replace_with);

  /* move pointer ahead for the length of the string to be replaced */
  str += strlen (to_replace);

  strcat (string2, str);

  string2[strlen (string2)] = '\0';

  return string2;
}
 
Old 12-19-2017, 09:54 AM   #2
GazL
LQ Guru
 
Registered: May 2008
Posts: 5,109
Blog Entries: 17

Rep: Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782
The thing that jumped out at me is that you use the 'str' pointer to store the result of your first calloc() and then the result from your strstr(). If strstr() doesn't find the string it will return NULL, overwriting the pointer to your calloc'd memory which you won't be able to free. You might want to consider using a separate pointer for each. (edit: I did notice you store it in addr_str, but you still return NULL to main in the case that strstr returns NULL. Anyway, it feels kind of klunky the way it's written now. )


I had a go myself and decided it was easier to loop over the strings myself rather than use strncat etc.

Code:
char *strrepl( char *dest, char *src, char *str, char *repl )
{
    char *s, *d, *p;
    
    s = strstr(src, str);
    if (s)
    {
	d = dest ;
	for (p = src ; p < s ; p++, d++)
	    *d = *p ;
	for (p = repl ; *p != '\0' ; p++, d++)
	    *d = *p ;
	for (p = s + strlen(str) ; *p != '\0' ; p++, d++)
	    *d = *p ;
	*d = '\0' ;
    } else
	strcpy(dest, src);
  
    return dest;
}
I also left the allocation of the buffer into which it writes to main() for simplicity.

I dare say it could be done better though. I'm really only a beginner myself when it comes to C.

Last edited by GazL; 12-19-2017 at 10:25 AM.
 
1 members found this post helpful.
Old 12-20-2017, 06:42 AM   #3
keefaz
LQ Guru
 
Registered: Mar 2004
Distribution: Slackware
Posts: 6,230

Rep: Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724
My take on the function although I like GazL's solution better
Code:
char*
strreplc (const char *orig, const char *to_replace, const char *replace_with)
{
	char *out;
	char *str = strstr(orig, to_replace);
	size_t orig_len = strlen(orig);

	/* return original string if to_replace not found */
	if (str == NULL) {
		out = (char*)malloc(orig_len+1);
		if (out == NULL) {
			fprintf(stderr, "Error: can not malloc\n");
			exit(EXIT_FAILURE);
		}

		memcpy(out, orig, orig_len+1);
		return out;
	}
	
	size_t to_len = strlen(to_replace);
	size_t with_len = strlen(replace_with);
	size_t out_len = orig_len - to_len + with_len;

	out = (char*)malloc(out_len+1);
	if (out == NULL) {
		fprintf(stderr, "Error: can not malloc\n");
		exit(EXIT_FAILURE);
	}
	
	int count = str - orig;
	char *outp = memcpy(out, orig, count);
	outp += count;
  
	memcpy(outp, replace_with, with_len);
	outp += with_len;
	str += to_len;
  
	memcpy(outp, str, orig_len - (str - orig));
	out[out_len] = '\0';

	return out;
}

Last edited by keefaz; 12-20-2017 at 07:26 AM.
 
2 members found this post helpful.
Old 12-20-2017, 02:34 PM   #4
GazL
LQ Guru
 
Registered: May 2008
Posts: 5,109
Blog Entries: 17

Rep: Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782
Quote:
Originally Posted by keefaz View Post
although I like GazL's solution better
I'm not so sure it's better. My version was designed around using a preexisting buffer. My thinking was that if you're going to have strlen() iterate over the string to find the length, then one may as well do the copy/replace while iterating. However, if you're going to have the function dynamically allocate an output buffer on the heap with malloc/calloc then you're going to need those strlen() results to allocate a buffer of the appropriate size beforehand: in which case you're probably better off using the memcpy() approach. Unless of course, you just over-allocate the buffer and don't worry about the wasted memory.
 
Old 12-20-2017, 06:00 PM   #5
keefaz
LQ Guru
 
Registered: Mar 2004
Distribution: Slackware
Posts: 6,230

Rep: Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724
Yes sure, but I flashed at the minimalistic approach

if compiling just with glibc, I would use gnu mempcpy() (returns pointer to the byte following the last written byte)
so code can be shortened a bit
Code:
#define _GNU_SOURCE
...

char*
strreplc (const char *orig, const char *to_replace, const char *replace_with)
{
    char *str = strstr(orig, to_replace);
    
    size_t to_len = strlen(to_replace);
    size_t with_len = strlen(replace_with);
    size_t orig_len = strlen(orig);
    size_t out_len = str == NULL ? orig_len : orig_len - to_len + with_len;
    
    char *out = malloc(out_len+1);
    if (out == NULL) {
        fprintf(stderr, "Error: can not malloc\n");
        exit(EXIT_FAILURE);
    }

    if (str != NULL) {
        char *outp = mempcpy(mempcpy(out, orig, str - orig), 
                             replace_with, with_len);
        str += to_len;
        mempcpy(outp, str, orig_len - (str - orig)+1);
    } else {
        /* return original string if to_replace not found */
        memcpy(out, orig, orig_len+1);
    }

    return out;
}

Last edited by keefaz; 12-23-2017 at 06:43 AM.
 
2 members found this post helpful.
Old 12-22-2017, 09:44 PM   #6
Andy Alt
Member
 
Registered: Jun 2004
Location: Minnesota, USA
Distribution: Slackware64-stable, Debian64 stable, LFS 7.1
Posts: 489

Original Poster
Rep: Reputation: 161Reputation: 161
Thank you for the ideas.

Gazl, when I run your code, it segfaults.

So you both are saying that my use of strlen() is not efficient because of the number of iterations strlen makes?
 
Old 12-23-2017, 06:05 AM   #7
keefaz
LQ Guru
 
Registered: Mar 2004
Distribution: Slackware
Posts: 6,230

Rep: Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724
The non freed memory allocation for str has to be fixed, reducing calls to strlen() is just a matter of optimization
 
1 members found this post helpful.
Old 12-23-2017, 07:43 AM   #8
GazL
LQ Guru
 
Registered: May 2008
Posts: 5,109
Blog Entries: 17

Rep: Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782
Quote:
Originally Posted by Andy Alt View Post
Gazl, when I run your code, it segfaults.
My version requires a pre-allocated char dest[] of appropriate size to hold the result, not a pointer like yours did, so you have to use it like this:
Code:
    char src[] = "A string to change." ;
    char dest[255] ;
  
    strrepl( dest, src, "string", "thing") ;
    printf("%s\n", dest) ;
if dest is just a pointer, it will most likely segfault.

By avoiding the malloc() I can also avoid the need to have strlen() loop over the strings.
 
1 members found this post helpful.
Old 12-23-2017, 02:23 PM   #9
keefaz
LQ Guru
 
Registered: Mar 2004
Distribution: Slackware
Posts: 6,230

Rep: Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724Reputation: 724
We could also allocate memory with a starting minimal size, then use a variable that stores the count of copied chars so far in each loop and test if its value becomes greater than allocated size, if yes reallocate with an extended size.

This way we save strlen() calls but with added complexity, test and reallocate management (check if it returns null etc)in each loop


Code:
...
size_t size_len = 100;
char *str = strstr(orig, to_replace);
size_t out_len = str == NULL ? strlen(orig) : size_len;

char *out = malloc(out_len+1);
if (out == NULL) 
	return NULL;

/* return original string if to_replace not found */
if (str == NULL) 
	return memcpy(out, orig, out_len+1);

char *outp = out;
const char *origp = orig;
size_t chars_so_far = 0;

while (origp < str) {
	if (chars_so_far++ >= out_len) {
		out_len += size_len;
		char *outpp = realloc(out, out_len+1);
		if (outpp == NULL) {
			free(out);
			return NULL;
		}
		outp = outpp + (outp - out);
	}
	*outp++ = *origp++;
}

/* and continue with 2 similar loops to complete the out string 
   call a final realloc to chars_so_far + 1 value to resize memory to out length
*/

Last edited by keefaz; 12-23-2017 at 02:33 PM.
 
2 members found this post helpful.
Old 07-30-2019, 08:17 PM   #10
Andy Alt
Member
 
Registered: Jun 2004
Location: Minnesota, USA
Distribution: Slackware64-stable, Debian64 stable, LFS 7.1
Posts: 489

Original Poster
Rep: Reputation: 161Reputation: 161
@GazL I'm using your code now
 
1 members found this post helpful.
Old 07-31-2019, 07:26 AM   #11
GazL
LQ Guru
 
Registered: May 2008
Posts: 5,109
Blog Entries: 17

Rep: Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782Reputation: 2782
I'm glad you found it useful.

Prompted by jsb's ffclifront thread, I now have some more string related functions: strtok_q() and strtok_qs(), so perhaps it's time to start bundling them into a libGazLStrings.

Last edited by GazL; 07-31-2019 at 07:54 AM.
 
1 members found this post helpful.
Old 07-31-2019, 02:35 PM   #12
Andy Alt
Member
 
Registered: Jun 2004
Location: Minnesota, USA
Distribution: Slackware64-stable, Debian64 stable, LFS 7.1
Posts: 489

Original Poster
Rep: Reputation: 161Reputation: 161
[QUOTE=GazL;6020112]I'm glad you found it useful. [quote]

Very useful for replacing variables with literals xD

Quote:
Prompted by jsb's ffclifront thread, I now have some more string related functions: strtok_q() and strtok_qs(), so perhaps it's time to start bundling them into a libGazLStrings.
Give me a ping if you want a collaborator xD
 
1 members found this post helpful.
  


Reply

Tags
c programming, function, strings


Thread Tools Search this Thread
Search this Thread:

Advanced Search

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off



Similar Threads
Thread Thread Starter Forum Replies Last Post
How to show selected string using grep from file and replace it with new input string prasad1990 Linux - Software 2 03-19-2015 08:02 AM
[SOLVED] request C source code review: rmw Andy Alt Programming 9 05-09-2013 06:16 AM
Replace part of string-delete last letter of it cgcamal Programming 15 06-18-2009 11:08 AM
problem in perl replace command with slash (/) in search/replace string ramesh_ps1 Red Hat 4 09-10-2003 01:04 AM

LinuxQuestions.org > Forums > Non-*NIX Forums > Programming

All times are GMT -5. The time now is 12:56 PM.

Main Menu
Advertisement
My LQ
Write for LQ
LinuxQuestions.org is looking for people interested in writing Editorials, Articles, Reviews, and more. If you'd like to contribute content, let us know.
Main Menu
Syndicate
RSS1  Latest Threads
RSS1  LQ News
Twitter: @linuxquestions
Facebook: linuxquestions Google+: linuxquestions
Open Source Consulting | Domain Registration