Fix a handful of compiler warnings #1862

AtariDreams · 2023-04-20T18:52:42Z

This fixes a couple of warnings raised by clang.

AtariDreams · 2023-07-06T15:40:49Z

Any updates on this?

kientzle · 2023-07-09T16:31:31Z

You say this fixes compiler warnings? What compilers have you tried, and what were the warnings you saw?

evelikov · 2023-07-14T07:41:53Z

Not a maintainer here, so take this with a pinch of salt:

On most, if not all platforms sizeof returns a size_t, although not many compilers will warn if the value is saved as an int. Having the commit message say, "Happens with compiler X, version Y and warning -Wfoobar" would be great.

That said, the commit does a bunch of unrelated changes at once. It would be better if they are split:

int -> size_t for sizeof
convert while loop into a for loop
drop some empty/white lines
dropping no-op returns
swap calloc argument order
removing duplicate includes - unistd.h , errno.h
drop misc changes that already landed with Prefer OR over addition #1915

The loop & whitespace ones seem superfluous, the rest are good fixes.

HTH

evelikov · 2023-10-11T11:02:32Z

@AtariDreams please update the PR to address the earlier review suggestions.

AtariDreams · 2023-10-11T17:21:46Z

@evelikov Done!

examples/minitar/minitar.c

evelikov · 2023-10-12T11:29:48Z

libarchive/archive_disk_acl_darwin.c

 		if (tacl_entry[i].permset != 0) {
 			archive_entry_acl_add_entry(entry,
 			    tacl_entry[i].type, tacl_entry[i].permset,
 			    tacl_entry[i].tag, -1, NULL);
 		}
 	}
-
-	return;


The return removal is good but should be part of the "remove redundant" PR.

libarchive/archive_disk_acl_darwin.c

libarchive/archive_disk_acl_freebsd.c

libarchive/archive_disk_acl_linux.c

libarchive/archive_read_support_filter_lzop.c

libarchive/archive_read_support_format_rar.c

This fixes a couple of warnings raised by clang.

evelikov · 2024-02-10T18:45:48Z

@AtariDreams I think the maintainers are looking for an answer to their earlier questions:

You say this fixes compiler warnings? What compilers have you tried, and what were the warnings you saw?

Personally it makes sense to also add the relevant -Wfoobar options to the autotools and cmake builds as part of this PR. This way it a) clearly illustrates/answers the above questions and b) reduces the odds of reintroducing similar changes in the future.

HTH o/

kientzle

There are some good changes here and some that seem to have no reason.

I also still want to know what the warnings were. I tried compiling the current libarchive code with a recent clang with -Wall and I don't see the warnings you're talking about.

kientzle · 2024-03-24T00:56:07Z

CMakeLists.txt

@@ -1090,7 +1090,7 @@ MACRO(CHECK_ICONV LIB TRY_ICONV_CONST)
    ENDIF(HAVE_ICONV_${LIB}_${TRY_ICONV_CONST})
    CMAKE_POP_CHECK_STATE()	# Restore the state of the variables
  ENDIF(NOT HAVE_ICONV)
-ENDMACRO(CHECK_ICONV TRY_ICONV_CONST)
+ENDMACRO()


Why did you remove the argument from this ENDMACRO() but no other? Was this one wrong somehow?

kientzle · 2024-03-24T01:02:04Z

libarchive/archive_read_open_fd.c

@@ -133,13 +133,13 @@ file_skip(struct archive *a, void *client_data, int64_t request)
 	struct read_fd_data *mine = (struct read_fd_data *)client_data;
 	int64_t skip = request;


skip here should be off_t since that's the type that the lseek calls expect.

kientzle · 2024-03-24T01:10:12Z

libarchive/archive_disk_acl_darwin.c

@@ -195,7 +195,7 @@ add_trivial_nfs4_acl(struct archive_entry *entry)
 	} else if ((mode & 0010) || (mode & 0001))
 		tacl_entry[1].permset |= eperm;

-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < sizeof(tacl_entry) / sizeof(tacl_entry[0]); i++) {


kientzle · 2024-03-24T01:11:05Z

libarchive/archive_read_disk_entry_from_file.c

@@ -793,7 +793,8 @@ setup_xattrs(struct archive_read_disk *a,
    struct archive_entry *entry, int *fd)
 {
 	int namespaces[2];
-	int i, res;
+	int res;
+	unsigned i;


unsigned here is not necessary.

libarchive/archive_read_support_format_rar.c

kientzle · 2024-03-24T01:12:51Z

libarchive/archive_write_disk_posix.c

@@ -706,7 +706,7 @@ _archive_write_disk_header(struct archive *_a, struct archive_entry *entry)
 			const void *attr_value;
 			size_t attr_size;
 			int i = archive_entry_xattr_reset(a->entry);
-			while (i--) {
+			while (i-- > 0) {


This is a good change that fixes a real warning on some compilers.

kientzle · 2024-03-24T01:13:06Z

libarchive/archive_write_disk_posix.c

@@ -4574,11 +4574,11 @@ set_xattrs(struct archive_write_disk *a)
 	struct archive_string errlist;
 	int ret = ARCHIVE_OK;
 	int i = archive_entry_xattr_reset(entry);
-	short fail = 0;
+	int fail = 0;


int is a better choice here.

kientzle · 2024-03-24T01:13:55Z

libarchive/archive_disk_acl_darwin.c

-static const int acl_nfs4_perm_map_size =
-    (int)(sizeof(acl_nfs4_perm_map)/sizeof(acl_nfs4_perm_map[0]));
+static const size_t acl_nfs4_perm_map_size =
+	(sizeof(acl_nfs4_perm_map) / sizeof(acl_nfs4_perm_map[0]));


This is an unnecessary style change: I generally prefer to use int unless there's a need to do otherwise. In this case, the map is a fixed list (just above) that has less than 20 entries. So int works perfectly fine and reduces the need for extra casting below.

AtariDreams force-pushed the Work branch 4 times, most recently from 4aadd92 to d2365be Compare April 20, 2023 21:05

AtariDreams force-pushed the Work branch from d2365be to 2d88d8b Compare July 6, 2023 15:41

AtariDreams force-pushed the Work branch 3 times, most recently from 2218fe1 to 43e9b97 Compare July 21, 2023 16:55

AtariDreams force-pushed the Work branch 3 times, most recently from 177b7ab to ca9c4b6 Compare October 11, 2023 04:59

AtariDreams force-pushed the Work branch 3 times, most recently from 3c45804 to 5daa616 Compare October 11, 2023 17:11

evelikov suggested changes Oct 12, 2023

View reviewed changes

AtariDreams force-pushed the Work branch from 5daa616 to 3d739de Compare October 16, 2023 22:35

AtariDreams requested a review from evelikov October 16, 2023 22:36

AtariDreams force-pushed the Work branch 3 times, most recently from 26a4a17 to 5f0588d Compare October 18, 2023 22:12

AtariDreams force-pushed the Work branch from 5f0588d to 5c48a61 Compare October 24, 2023 15:49

Fix a handful of compiler warnings

9a6857d

This fixes a couple of warnings raised by clang.

AtariDreams force-pushed the Work branch from 5c48a61 to 9a6857d Compare February 6, 2024 17:05

kientzle reviewed Mar 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a handful of compiler warnings #1862

Fix a handful of compiler warnings #1862

AtariDreams commented Apr 20, 2023 •

edited

AtariDreams commented Jul 6, 2023

kientzle commented Jul 9, 2023

evelikov commented Jul 14, 2023

evelikov commented Oct 11, 2023

AtariDreams commented Oct 11, 2023

evelikov Oct 12, 2023

evelikov commented Feb 10, 2024

kientzle left a comment

kientzle Mar 24, 2024

kientzle Mar 24, 2024

kientzle Mar 24, 2024

kientzle Mar 24, 2024

kientzle Mar 24, 2024

kientzle Mar 24, 2024

kientzle Mar 24, 2024

		@@ -133,13 +133,13 @@ file_skip(struct archive a, void client_data, int64_t request)
		struct read_fd_data mine = (struct read_fd_data )client_data;
		int64_t skip = request;

Fix a handful of compiler warnings #1862

Are you sure you want to change the base?

Fix a handful of compiler warnings #1862

Conversation

AtariDreams commented Apr 20, 2023 • edited

AtariDreams commented Jul 6, 2023

kientzle commented Jul 9, 2023

evelikov commented Jul 14, 2023

evelikov commented Oct 11, 2023

AtariDreams commented Oct 11, 2023

Choose a reason for hiding this comment

evelikov commented Feb 10, 2024

kientzle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtariDreams commented Apr 20, 2023 •

edited