-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C89 support #1616
base: master
Are you sure you want to change the base?
C89 support #1616
Conversation
…ompliant features; [{cpio/cpio.h,**/*.c}] C90 conformance
My 2cents in case they matter: Time would be better spend fixing said compiler or transitioning to another, instead of pushing technical burden to open-source projects. |
@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. |
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. |
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. |
@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. |
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. |
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. |
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above three seem rather uncommon/new. I would suggest doing a compile-time check if they're present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't compile on strict C89 mode without these
@@ -114,7 +114,7 @@ enum { | |||
OPTION_QUIET, | |||
OPTION_UUENCODE, | |||
OPTION_VERSION, | |||
OPTION_ZSTD, | |||
OPTION_ZSTD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: const to stay in line with the other const
additions through the patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add empty line between declarations and statements.
ZSTD_inBuffer in; | ||
in.src = src; | ||
in.size = length; | ||
in.pos = 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the out
instance above - please use the more common one of {}
{0}
or memset
.
@@ -83,6 +83,38 @@ test_filter_or_format(enabler enable) | |||
|
|||
DEFINE_TEST(test_archive_read_support) | |||
{ | |||
int format_codes[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: const
unsigned long set, clear, flag; | ||
|
||
flag = 0; | ||
unsigned long set, clear, flag=0; | ||
|
||
PROLOGUE("test_read_format_rar5_fileattr.rar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary changes - please drop.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated size_t
-> ssize_t
- please drop and keep a separate patch, with clear commit message explaining why.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto - drop or separate patch please.
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) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use two variables crc_foo
and crc
. The code looks quite uncommon and "ugly" IMHO
|
||
const char *zip_end = zip_buff + size; | ||
/* Fix incompatible-pointer-types-discards-qualifiers */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
data, | ||
central_directory, | ||
data_descriptor, | ||
local_folder_header; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holly strange indentations - please use const char *foo, *bar
or const char *foo;\nconst char *bar;
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. |
@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). |
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. |
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. |
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? |
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. |
Should open the door to porting libarchive to estoric and old platforms, and with more FFI tooling