C89 support #1616

SamuelMarks · 2021-11-17T03:31:18Z

Should open the door to porting libarchive to estoric and old platforms, and with more FFI tooling

…ompliant features; [{cpio/cpio.h,**/*.c}] C90 conformance

evelikov · 2021-11-19T21:56:30Z

My 2cents in case they matter:
If a platform does not have, even partial C99 support it's not worth supporting in 2021(2022) and beyond IMHO.

Time would be better spend fixing said compiler or transitioning to another, instead of pushing technical burden to open-source projects.

SamuelMarks · 2021-11-21T04:13:45Z

@evelikov I agree, if the changes required were nontrivial. But they are quite trivial (just look at the diff). At the juncture where features >C89 are needed, then your argument could hold weight.

evelikov · 2021-11-21T11:07:50Z

This is focusing on the changes themselves, without looking at the bigger picture. In similar fashion one can also say that libarchive can go K&R - the changes will also be trivial :-P

Your point has merit, if libarchive was in deep maintenance mode with no code being added. As new code gets added and refactored the onus (on both developers and maintainers) grows.

Looking from another angle - if it's so trivial - just carry it downstream? I'm not particularly excited about restricting myself to C89 hence rewriting #1603 and any follow-ups.

evelikov · 2021-11-21T11:11:06Z

As you can see from the CI failure, libarchive uses third-party projects written in C99. So the crusade of porting the world to C89 is admirable, yet it may scale better if fixed on your end.

SamuelMarks · 2021-11-21T14:20:37Z

@evelikov If this project is interested in merging this PR, then lets convert this PR to draft and I'll start contributing C89 support to all its third-party dependencies.

kientzle · 2021-11-21T18:48:54Z

I don't have a strong opinion either way. For compatibility with Visual C++ (which did not fully support C99 until fairly recently), libarchive is already "mostly" C90 compliant, as you found out.

I'm curious what specific system and/or compiler motivated this? My experience has been that certain C99 features (especially the ability to intermix declarations) are pretty widely supported, even in compilers that lack full support.

evelikov · 2021-11-23T22:10:59Z

Can I suggest that instead of forcing C89/90 onto everyone, that we warn/error on features that your setup does not support.

I'll put some specific suggestions in a moment.

evelikov · 2021-11-23T22:12:17Z

CMakeLists.txt

+  SET(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -Wno-long-long")
+  SET(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -Wno-extra-semi")
+  SET(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -Wno-language-extension-token")
+  SET(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -Wno-overlength-strings")


The above three seem rather uncommon/new. I would suggest doing a compile-time check if they're present.

It won't compile on strict C89 mode without these

evelikov · 2021-11-23T22:13:19Z

cpio/cpio.h

@@ -114,7 +114,7 @@ enum {
 	OPTION_QUIET,
 	OPTION_UUENCODE,
 	OPTION_VERSION,
-	OPTION_ZSTD,
+	OPTION_ZSTD


I would leave the trailing commas, unless your compiler really does not like them. As you saw with the MacOS build - they are very common.

evelikov · 2021-11-23T22:15:33Z

libarchive/archive_read_support_filter_lz4.c

@@ -344,31 +344,33 @@ lz4_filter_read(struct archive_read_filter *self, const void **p)
 			state->eof = 1;
 			*p = NULL;
 			return (0);
-		}
-		uint32_t number = archive_le32dec(read_buf);


Here and through the rest of the patch - do not re-indent hunks of code, just move the declaration further up. As-is it ~~moves~~ touches 400+ lines of code, where a mere 10-20 will be enough.

You'd want to enable -Wdeclaration-after-statement to catch these issues.

evelikov · 2021-11-23T22:17:40Z

libarchive/archive_read_support_filter_zstd.c

-	out = (ZSTD_outBuffer) { state->out_block, state->out_block_size, 0 };
+	out.dst = state->out_block;
+	out.pos = state->out_block_size;
+	out.size = 0;


The original code will always zero fill any struct fields that are not mentioned. I would use either out = {}, out = { 0 } or memset() - whichever is more common in the project.

evelikov · 2021-11-23T22:21:59Z

libarchive/archive_read_support_format_zip.c

@@ -1168,20 +1168,18 @@ zip_read_local_file_header(struct archive_read *a, struct archive_entry *entry,
 	archive_entry_set_atime(entry, zip_entry->atime, 0);

 	if ((zip->entry->mode & AE_IFMT) == AE_IFLNK) {
-		size_t linkname_length;
+		size_t linkname_length = (size_t)zip_entry->compressed_size;


Nit: const to stay in line with the other const additions through the patch?

evelikov · 2021-11-23T22:22:43Z

libarchive/archive_write_add_filter_zstd.c

@@ -207,13 +207,12 @@ archive_compressor_zstd_options(struct archive_write_filter *f, const char *key,
 		data->compression_level = level;
 		return (ARCHIVE_OK);
 	} else if (strcmp(key, "threads") == 0) {
-		int threads = atoi(value);
+		const int threads = atoi(value);
+		const int minimum = 0;


Nit: add empty line between declarations and statements.

evelikov · 2021-11-23T22:23:33Z

libarchive/archive_write_add_filter_zstd.c

+	ZSTD_inBuffer in;
+	in.src = src;
+	in.size = length;
+	in.pos = 0;



Like the out instance above - please use the more common one of {} {0} or memset.

evelikov · 2021-11-23T22:24:02Z

libarchive/test/test_archive_read_support.c

@@ -83,6 +83,38 @@ test_filter_or_format(enabler enable)

 DEFINE_TEST(test_archive_read_support)
 {
+	int format_codes[] = {


Nit: const

evelikov · 2021-11-23T22:24:25Z

libarchive/test/test_read_format_rar5.c

-	unsigned long set, clear, flag;
-
-	flag = 0;
+	unsigned long set, clear, flag=0;

 	PROLOGUE("test_read_format_rar5_fileattr.rar");


Unnecessary changes - please drop.

evelikov · 2021-11-23T22:25:45Z

libarchive/test/test_short_writes.c

        struct checker *checker = client_data;
-        size_t to_write = length < 100 ? length : 100;
-        size_t new_len = checker->shortbuf_len + to_write;
+        const ssize_t to_write = length < 100 ? (ssize_t)length : 100;


Unrelated size_t -> ssize_t - please drop and keep a separate patch, with clear commit message explaining why.

evelikov · 2021-11-23T22:26:08Z

libarchive/test/test_short_writes.c

        struct checker *checker = client_data;
-        size_t to_write = length;
-        size_t new_len = checker->fullbuf_len + to_write;
+        const ssize_t to_write = (ssize_t)length;


Ditto - drop or separate patch please.

evelikov · 2021-11-23T22:27:04Z

libarchive/test/test_write_format_zip_entry_size_unset.c

-	crc = bitcrc32(crc, file_data2, sizeof(file_data2));
+	const unsigned long crc = bitcrc32(
+			bitcrc32(0, file_data1, sizeof(file_data1)), file_data2, sizeof(file_data2)
+	);


Just use two variables crc_foo and crc. The code looks quite uncommon and "ugly" IMHO

evelikov · 2021-11-23T22:30:05Z

libarchive/test/test_write_format_zip_entry_size_unset.c


 	const char *zip_end = zip_buff + size;
+	/* Fix incompatible-pointer-types-discards-qualifiers */


From quick look, I cannot see where/why that is the case. If needed please consider fixing the API to use const char * or if that's not possible use local casting.

The union seems like a pretty gross hack.

evelikov · 2021-11-23T22:30:49Z

libarchive/test/test_write_format_zip_entry_size_unset.c

+	                data,
+					central_directory,
+					data_descriptor,
+					local_folder_header;


Holly strange indentations - please use const char *foo, *bar or const char *foo;\nconst char *bar;

crrodriguez · 2022-02-06T03:39:56Z

I have to ask... which compiler is so broken that this is needed at all? not sure what you are trying to accomplish here..any mainstream compiler of the last 10-15 years is capable of compiling libarchive.. and any new or esoteric system will port clang to said niche system or maybe contribute a gcc target. because building a new compiler from the scratch ain't going to make dollars&sense whatsoever.

SamuelMarks · 2022-02-06T15:59:19Z

@crrodriguez Everything I write in C targets C89. This ensure maximum portability.

Which is great for targeting old and esoteric systems, porting to new targets (e.g., WASM), and weird new systems like http://www.pdos.org… as well as supporting every languages FFI layer (many of which only support C89) and being able to benchmark across them (e.g., different libc implementations).

ljdarj · 2022-02-24T17:20:38Z

I'm pretty sure it's not possible to port libarchive to be strictly C89 compliant without far too much effort for the reward because libarchive's external identifiers are not only too long, they are generally distinguished by their suffixes. And to be strictly C89 compliant, they would have needed to be distinguished on their first 6 characters.

zhengtu1122 · 2023-05-26T22:03:07Z

My experience has been that certain C99 features (especially the ability to intermix declarations) are pretty widely supported, even in compilers that lack full support.

kientzle · 2024-04-05T18:22:59Z

Which is great for targeting old and esoteric systems ...

Which specific systems have you tried porting libarchive to and failed? I'm not particularly interested in C89 support for its own sake. I know people using C++17 on WASM, so I would be surprised to hear that C89 was required there. While PDOS itself claims to be written in C90, the toolchain for it appears to be based on GCC9.4, which definitely supports C99. What specific targets are you trying to work with?

jsonn · 2024-04-07T00:15:14Z

I'm very much against the restriction to C90, even a C90 with 64bit extensions. I also find many of the changes here annoying regressions, e.g. trailing comma at the end of an enum is something I want very much as it reduces diff churn.

[CMakeLists.txt] Set C standard to C90; ignore some pedantic C90 nonc…

1bb8bf6

…ompliant features; [{cpio/cpio.h,**/*.c}] C90 conformance

evelikov reviewed Nov 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C89 support #1616

C89 support #1616

SamuelMarks commented Nov 17, 2021

evelikov commented Nov 19, 2021

SamuelMarks commented Nov 21, 2021

evelikov commented Nov 21, 2021

evelikov commented Nov 21, 2021

SamuelMarks commented Nov 21, 2021

kientzle commented Nov 21, 2021

evelikov commented Nov 23, 2021

evelikov Nov 23, 2021

SamuelMarks Nov 24, 2021

evelikov Nov 23, 2021 •

edited

evelikov Nov 23, 2021 •

edited

evelikov Nov 23, 2021

evelikov Nov 23, 2021

zhengtu1122 May 26, 2023

evelikov Nov 23, 2021

evelikov Nov 23, 2021

evelikov Nov 23, 2021

evelikov Nov 23, 2021

evelikov Nov 23, 2021

evelikov Nov 23, 2021

evelikov Nov 23, 2021 •

edited

evelikov Nov 23, 2021

evelikov Nov 23, 2021 •

edited

crrodriguez commented Feb 6, 2022

SamuelMarks commented Feb 6, 2022

ljdarj commented Feb 24, 2022

zhengtu1122 commented May 26, 2023

kientzle commented Apr 5, 2024

jsonn commented Apr 7, 2024


		const char *zip_end = zip_buff + size;
		/* Fix incompatible-pointer-types-discards-qualifiers */

C89 support #1616

Are you sure you want to change the base?

C89 support #1616

Conversation

SamuelMarks commented Nov 17, 2021

evelikov commented Nov 19, 2021

SamuelMarks commented Nov 21, 2021

evelikov commented Nov 21, 2021

evelikov commented Nov 21, 2021

SamuelMarks commented Nov 21, 2021

kientzle commented Nov 21, 2021

evelikov commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evelikov Nov 23, 2021 • edited

Choose a reason for hiding this comment

evelikov Nov 23, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evelikov Nov 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evelikov Nov 23, 2021 • edited

Choose a reason for hiding this comment

crrodriguez commented Feb 6, 2022

SamuelMarks commented Feb 6, 2022

ljdarj commented Feb 24, 2022

zhengtu1122 commented May 26, 2023

kientzle commented Apr 5, 2024

jsonn commented Apr 7, 2024

evelikov Nov 23, 2021 •

edited

evelikov Nov 23, 2021 •

edited

evelikov Nov 23, 2021 •

edited

evelikov Nov 23, 2021 •

edited