Return to Answer

Clarify that redundant sizeof is good practice

edited Aug 27, 2024 at 6:43

87.4k
14
104
322

Multiplication by sizeof *buff ismay seem redundant given that sizeof (char) is necessarily 1. However, it's always good practice to tie size calculations with the size of the type like that (though I'd change the parens to (sizeof *buff) for clarity), as it can help facilitate code re-use, e.g. with wide strings.

Multiplication by sizeof *buff is redundant given that sizeof (char) is necessarily 1.

Multiplication by sizeof *buff may seem redundant given that sizeof (char) is necessarily 1. However, it's always good practice to tie size calculations with the size of the type like that (though I'd change the parens to (sizeof *buff) for clarity), as it can help facilitate code re-use, e.g. with wide strings.

C still allows non-const string literals

Source Link

edited Aug 22, 2024 at 14:46

Toby Speight

edited Aug 22, 2024 at 14:46

Toby Speight

87.4k
14
104
322

The arguments should be const char* so that we can call with string literals (as indeed we do) without swamping us with compiler warnings (if you've not enabled such warnings, I recommend you do - they can catch serious bugs).

The arguments should be const char* so that we can call with string literals (as indeed we do).

Source Link

answered Aug 22, 2024 at 8:39

Toby Speight

answered Aug 22, 2024 at 8:39

Toby Speight

87.4k
14
104
322

static void exit_with_message(char *func_name, char *ptr_name, char *message)
{
 printf("(%s)-(%s): %s\n", func_name, ptr_name, message);
 exit(1);
}

We should annotate this function with [[noreturn]].

The arguments should be const char* so that we can call with string literals (as indeed we do).

Error messages should go to standard error stream:

 fprintf(stderr, "(%s)-(%s): %s\n", func_name, ptr_name, message);

If we include <stdlib.h>, we can use a more descriptive exit status:

 exit(EXIT_FAILURE);

void freestr(String *w)
{
 if (!w->s)
 return;
 free(w->s);
 if (w->s != NULL)
 w->s = NULL;
}

Both of the tests are unnecessary - if we remove them, the behaviour is unchanged. However, it may be a good idea to check that w is not null before accessing its members:

void freestr(String *w)
{
 if (!w) {
 return;
 }
 free(w->s);
 w->s = NULL;
 w->length = 0;
}

size_t i, j;
String newString;

Prefer to defer declaration of locals to the point where they can be initialised. It's no longer necessary to declare all variables at the start of their scope.

char *buff = malloc(sizeof(*buff) * (w1->length + w2->length + 1));

Multiplication by sizeof *buff is redundant given that sizeof (char) is necessarily 1.

After making the code const-correct, fix the memory leaks:

gcc-14 -std=c23 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wmissing-braces -Wconversion -Wstrict-prototypes -fanalyzer 293412.c -o 293412
293412.c: In function ‘test_newstr_length’:
293412.c:241:1: warning: leak of ‘<U7510>.s’ [CWE-401] [-Wanalyzer-malloc-leak]
 241 | }
 | ^
 ‘main’: events 1-2
 |
 | 374 | int main()
 | | ^~~~
 | | |
 | | (1) entry to ‘main’
 | 375 | {
 | 376 | tests();
 | | ~~~~~~~
 | | |
 | | (2) calling ‘tests’ from ‘main’
 |
 +--> ‘tests’: events 3-4

(much snipped to fit within max answer length)

 | | (245) following ‘false’ branch...
 |......
 | 214 | return 1;
 | | ~
 | | |
 | | (248) ...to here
 |
 <------+
 |
 ‘test_newstr_actual_string’: event 249
 |
 | 254 | assert(issame(newstr("").s, ""));
 | | ^
 | | |
 | | (249) returning to ‘test_newstr_actual_string’ from ‘issame’
 |
 ‘test_newstr_actual_string’: event 250
 |
 | 254 | assert(issame(newstr("").s, ""));
 | | ^~~~~~
 | | |
 | | (250) following ‘false’ branch...
 |
 ‘test_newstr_actual_string’: events 251-252
 |
 | 255 | }
 | | ^
 | | |
 | | (251) ...to here
 | | (252) ‘<Uf630>.s’ leaks here; was allocated at (236)
 |

valgrind --leak-check=full ./293412 
=わ=わ2357414== Memcheck, a memory error detector
==2357414== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==2357414== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==2357414== Command: ./293412
==2357414== 
----- STRING TESTS PASS ------
==2357414=わ=わ 
=わ=わ2357414== HEAP SUMMARY:
==2357414== in use at exit: 1,243 bytes in 20 blocks
==2357414== total heap usage: 2,047 allocs, 2,027 frees, 504,947 bytes allocated
==2357414=わ=わ 
=わ=わ2357414=わ=わ 1 bytes in 1 blocks are definitely lost in loss record 1 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109BE6: test_newstr_actual_string (293412.c:254)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 4 bytes in 1 blocks are definitely lost in loss record 2 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x1093AD: concatRaw (293412.c:120)
==2357414== by 0x109EE5: test_concatRaw (293412.c:302)
==2357414== by 0x10A010: test_concat (293412.c:325)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 6 bytes in 1 blocks are definitely lost in loss record 3 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x1098A3: test_newstr_length (293412.c:235)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 6 bytes in 1 blocks are definitely lost in loss record 4 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x1098E0: test_newstr_length (293412.c:236)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 6 bytes in 1 blocks are definitely lost in loss record 5 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x10991D: test_newstr_length (293412.c:237)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 6 bytes in 1 blocks are definitely lost in loss record 6 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x10995A: test_newstr_length (293412.c:238)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 6 bytes in 1 blocks are definitely lost in loss record 7 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109A18: test_newstr_actual_string (293412.c:246)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 6 bytes in 1 blocks are definitely lost in loss record 8 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109AFF: test_newstr_actual_string (293412.c:249)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 7 bytes in 1 blocks are definitely lost in loss record 9 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109AB2: test_newstr_actual_string (293412.c:248)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 7 bytes in 1 blocks are definitely lost in loss record 10 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109B4C: test_newstr_actual_string (293412.c:250)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 7 bytes in 1 blocks are definitely lost in loss record 11 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109B99: test_newstr_actual_string (293412.c:251)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 14 bytes in 1 blocks are definitely lost in loss record 12 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109997: test_newstr_length (293412.c:239)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 14 bytes in 1 blocks are definitely lost in loss record 13 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x1093AD: concatRaw (293412.c:120)
==2357414== by 0x109DFD: test_concatRaw__basic (293412.c:288)
==2357414== by 0x10A006: test_concat (293412.c:323)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 21 bytes in 1 blocks are definitely lost in loss record 14 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x10A2F0: test_charat (293412.c:354)
==2357414== by 0x10A412: tests (293412.c:370)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 22 bytes in 1 blocks are definitely lost in loss record 15 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x1099D4: test_newstr_length (293412.c:240)
==2357414== by 0x109C30: test_newstr (293412.c:260)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 22 bytes in 1 blocks are definitely lost in loss record 16 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x109D00: test_concat__basic (293412.c:275)
==2357414== by 0x10A001: test_concat (293412.c:322)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 27 bytes in 1 blocks are definitely lost in loss record 17 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x1094EB: newstr (293412.c:177)
==2357414== by 0x109A65: test_newstr_actual_string (293412.c:247)
==2357414== by 0x109C35: test_newstr (293412.c:261)
==2357414== by 0x10A403: tests (293412.c:367)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 30 bytes in 1 blocks are definitely lost in loss record 18 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x109C81: test_concat__basic (293412.c:271)
==2357414== by 0x10A001: test_concat (293412.c:322)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 30 bytes in 1 blocks are definitely lost in loss record 19 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x109D7F: test_concat__basic (293412.c:279)
==2357414== by 0x10A001: test_concat (293412.c:322)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414=わ=わ 1,001 bytes in 1 blocks are definitely lost in loss record 20 of 20
==2357414== at 0x4842808: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2357414== by 0x109241: concat (293412.c:76)
==2357414== by 0x1093AD: concatRaw (293412.c:120)
==2357414== by 0x109FAC: test_concatRaw__stress (293412.c:314)
==2357414== by 0x10A00B: test_concat (293412.c:324)
==2357414== by 0x10A408: tests (293412.c:368)
==2357414== by 0x10A41E: main (293412.c:376)
==2357414=わ=わ 
=わ=わ2357414== LEAK SUMMARY:
==2357414== definitely lost: 1,243 bytes in 20 blocks
==2357414== indirectly lost: 0 bytes in 0 blocks
==2357414== possibly lost: 0 bytes in 0 blocks
==2357414== still reachable: 0 bytes in 0 blocks
==2357414== suppressed: 0 bytes in 0 blocks
==2357414=わ=わ 
=わ=わ2357414== For lists of detected and suppressed errors, rerun with: -s
==2357414== ERROR SUMMARY: 20 errors from 20 contexts (suppressed: 0 from 0)

And everything in Loki's answer.

lang-c