char current = fgetc (text_file_pointer); discards the difference between an error (int -1) and a char with value -1. Use int to hold fgetc return values. As the man page explains, EOF is not a character (it can't be represented by unsigned
char). It is instead a sentinel value outside of the valid range
for valid characters.
If a 0xFF byte is the first non-ASCII, your loop could return success thinking it reached EOF. That's assuming you're compiling for x86, where char is signed in the usual ABIs, so (char)-1 sign-extends to (int)-1. For systems with unsigned char (like ARM), (char)EOF == EOF would zero-extend the char to 0x000000FF which is not-equal to 0xFFFFFFFF (-1), so you'd be stuck in an infinite loop with fgetc returning EOF. (Glibc's EOF is -1; I was looking at how this compiled for x86 GNU/Linux.)
Use int current = fgetc(fp); (Or better use fread to read more bytes at a time.)
Compiler options
--std=c2x is the upcoming C23 standard, which gives you bool, true, and false without even having to include <stdbool.h> to use them. So generally use those instead of custom macros, unless you've looked at the compiler's assembly output and want to help the compiler save instructions by returning 0 vs. a non-zero value that might not be 1 (true). Most parts of most programs aren't that performance-sensitive, and inlining of small functions usually makes this problem negligible if it existed at all.
gcc main.c -Wall -Wextra --pedantic --std=c2x -D_GNU_SOURCE -Ofast -march=corei7 -mtune=corei7
Why first-gen corei7? That's Nehalem from 2008, unlikely to be what you want to tune for. If you have a Nehalem, use -march=native or -march=nehalem in GCC that isn't ancient. As Intel kept the Core i3/5/7 branding for many generations of different microarchitectures, it became a less and less useful name for a compiler option.
-march=x86-64-v2 is an SSE4.2 feature set (with generic tuning), like Nehalem and Silvermont-family before Alder Lake E-cores (Gracemont). -march=x86-64-v3 is basically Haswell, AVX2+FMA+BMI2. https://en.wikipedia.org/wiki/X86-64#Microarchitecture_levels . You can also use real microarch names with GCC's -march option, like -march=skylake, much more meaningful than -march=corei7-avx or core-avx2.
-march=x86-64-v2 -mtune=intel is also a thing you might want to try if you want to make code that will run on any "core i7" branded CPU (including the early generation before Sandybridge without AVX), and tuned for Intel CPUs in general, not specifically for the oldest.
-march=x implies -mtune=x so that's redundant when they're the same, but some people prefer to be explicit.
-Ofast can do some surprising stuff: apart from -ffast-math, it also enables -fallow-store-data-races which can turn if (x) arr[i] = y; into arr[i] = x ? y : arr[i]; which isn't thread-safe if another thread is also storing to the array. That's fine for this program, but just beware that it's not in general even safe in ways other than changing FP results.
check_for_non_ascii behaviour and naming
Your check_for_non_ascii returns true for non-empty ASCII-only files. This seems backwards relative to the function name. You actually called your constants SUCCESS and FAIL so these sort of make sense in the context of the whole program you're writing. But if you look at this function's task in isolation (which is a good idea when naming), it's just asking whether or not a file contains any non-ASCII characters. Neither result is a "failure"; that would be an I/O error. (Which it doesn't have any way to report; perhaps a third output state like negative for error vs. 0 (false) vs. positive (true).)
Also, this function seems to be responsible for detecting empty files as well. So it's actually checking that a file consists of 1 or more ASCII characters. You already lstat each file before even opening it, so you can check for size == 0 before even opening it and remove that responsibility. (You're only doing this for regular files anyway, not symlinks or anything else.) If the d_type member of struct dirent is available and not DT_UNKNOWN, though, you could avoid those lstat calls but wouldn't get size information. And the file could get truncated between check and open, so maybe best to just operate in terms of reading until EOF. Another TOCTOU race possibility is that a regular file is replaced by something else, like a pipe, or symlink to /dev/zero (which would make this program run forever). fstat after opening would be another way to at least check that what we've opened is a regular file. It couldn't avoid following a symlink to a different regular file, though. For that, you'd need open(2) with O_NOFOLLOW (POSIX 2008) instead of C stdio fopen.
Anyway, naming is tricky, but you want a name that makes code calling it easy to read, like if (!is_pure_ascii(fp)) { unlink(name); }, or if (empty_or_nonascii(fp)) { unlink(name); }
Efficiently checking for non-ASCII
Your check_for_non_ascii function is slow, like hundreds of times slower than necessary (at least the user-space part) for ASCII files of at least a few KiB in size. fgetc is much slower than fgetc_unlocked (concurrent use of the same FILE* is required to be thread-safe), but neither would give the compiler the opportunity to do anything useful with your -Ofast -march=corei7 options, and even just function-call overhead per character is not good, plus it will have to update things in the FILE struct pointed-to by your FILE*, and even fgetc_unlocked doesn't inline so the compiler can't just optimize them into registers.
To check a whole buffer for ASCII (after fread), we just need to check that none of them have their high bit set. Probably the most efficient way to do that is to OR all the bytes together (which compilers can use SIMD instructions for to process 16 or 32 at a time) and then check the MSB of the result.
This optimization is based on details of what we're checking for, specifically that US-ASCII is a 7-bit character set, padded to 8-bit in modern computers. If you want to keep things more generic, you could potentially write problem |= !isascii(buf[i]); to have it unconditionally do a bunch of whatever kind of check. But it probably won't auto-vectorize the locale-dependent table lookups for functions like isalpha. The return value of isalpha for example is an int where the non-zero bit (if there is one) might be outside the low 8 bits, but ! booleanizes. If doing ok &= isalpha(buf[i]) or something, you'd need int ok = -1; not char ok, but then you'd be assuming that all calls to is... have their non-zero bit (if any) in the same place, which is probably true but not guaranteed. For example many of glibc's is... functions like isupper or isalpha use locale-dependent lookup tables of 16-bit groups of flags, so isalnum and isdigit are just different bits of the same word indexed by the 8-bit character. Fun fact: EOF is a valid input to those functions, as well as as any (unsigned char)c, so the legal value-range of the int character input is usually -1 .. 255.
Current compilers fail to auto-vectorize loops with an early-out condition, which is why we want to write the source to unconditionally process some number of bytes.
If we look at the asm from glibc's isascii compiled with GCC or Clang, we see it's using test al, al / jns which jumps according to the sign flag set by test according to the MSB of the byte. So your isascii check isn't excluding control characters or even NUL '\0' bytes. If you want that, do it manually. isprint checks for "printable characters including space", but that might just be 0x20 (space) to 0x7E ('~'), not newline. So maybe isprint(c) | isspace(c) if you want to be picky about files you accept. This probably defeats auto-vectorization, since even if the compiler could assume the C locale, it's almost certainly not going to invent pcmpistri instructions to check 16 bytes at once for being in one of the valid ranges.
Choice of buffer size: some degree of early-out is good since most non-text files will have a high byte fairly early. 00 bytes are more common early in binary files like ELF executables, but we're accepting those, and the first byte of the ELF magic number at the start of the file is 7F. (hexdump -C foo | less and search for [0-F] to check the leading digit for MSB set).
I/O from disk is normally going to work in at least 4K chunks. (And reading from the kernel should be done at least 8K at a time; system call overhead is high, which is why the C stdio library does buffering for us.) Just reading 8K even on the first fread is very reasonable on modern computers, the actual copying goes fast compared to overhead like page faults. That's just two pages on x86, maybe fewer on some ISAs, and comfortably fits in L1d cache size so it'll be fast to loop over it. If you were going to switch sizes after finding the first 4K was ok, 64KiB is a good size to amortize read system call overhead vs. data still fitting in L2 cache after the kernel basically does a memcpy from the pagecache inside the read system call. Or mmap is very good for reading files this way, especially when they're hot in disk cache, but it's more complicated to use.
C stdio fread hopefully gets out of the way when doing large even-sized reads, but I notice you're calling POSIX lstat so you could be using open (or openat), read, close, and unlink (or unlinkat) if you wanted to make other parts of the code dependent on POSIX instead of just ISO C.
static
bool is_ascii_buffer(unsigned char *buf, size_t len){
unsigned char setbits = 0;
for (size_t i = 0 ; i<len ; i++){
setbits |= buf[i];
}
// better would be OR reduce to one vector then _mm_movemask_epi8 to get all their sign bits with one step.
// But not even clang is smart enough to realize that for either way of writing this check.
return !(setbits & 0x80);
//return setbits < 0x80;
}
int empty_or_nonascii(FILE * text_file_pointer)
{
#define BUFSIZE 8192
static unsigned char buf[BUFSIZE]; // on the stack (non-static) would also be ok for this size
bool is_empty = true;
size_t len;
while( (len = fread(buf, 1, BUFSIZE, text_file_pointer)) != 0 ) {
is_empty = false; // alternative to hoisting the first read out of the loop to special-case empty files. The compiler might transform the loop on its own
if (is_ascii_buffer(buf, len)) {
return 1;
}
}
// Unlike POSIX read, stdio fread only returns short on EOF or error
// It also doesn't distinguish EOF vs. error in its return value, also unlike read
if (ferror(text_file_pointer)) {
return -1; // also non-zero if caller just checks for 0 vs. non-zero
}
// feof(text_file_pointer) must be true at this point
return is_empty;
#undef BUFSIZE
}
Doing 8K fread calls but checking in chunks of 1K, 2K, or 4K would also make sense. Compiler auto-vectorization is not wonderful, with reducing to scalar taking many shuffle steps instead of one pmovmskb. And it has to check for odd sizes and inefficiently uses scalar or instead of a partially-overlapping vector (which would be fine because of how bitwise or works). Although that would be less of a problem for round sizes in an outer loop over this. Anyway, that cleanup should be a small part of the total time for zipping through text files of a few KiB or larger, and pretty trivial either way for small files.
I haven't tried to go all out in optimizing; this is still highly readable yet the compiler is able to come pretty close to what one might do by hand, doing most of the important stuff to make it fast in most cases. ( Although GCC still shoots itself in the foot by using the same vector register twice inside the loop, creating a latency bottleneck that will stop it from running at 2 vector loads per clock, except with the help of out-of-order exec.) Godbolt
If malloc fails, just give up
This source line is pretty long, although it's obvious what the goal is so as long as it works, I guess cramming it onto one line avoids distracting the reader from the important part. But I'd make it just an if without the retry.
while (((result = malloc(sizeof(char) * (directory_length + separator_length + file_name_length + 1))) == NULL) && (++count < ALLOCATION_ATTEMPT_LIMIT))
sleep(1);
If the system is so short of memory that a small malloc fails, probably best to just exit (after maybe printing something about current progress, see other answers). In practice on some OSes (e.g. Linux), allocation basically always succeeds, but the kernel might have to kill a process if it doesn't have the physical pages (+ swap) to back up the allocated virtual memory when processes later touch it. This is called "overcommit". Windows is apparently less aggressive about overcommit so allocation might actually fail, but the standard behaviour memory is to exit in that case, since that will free up memory which is vital for the overall system right now.
The rest of concat_file_path is well designed: after you do 3x strlen to get a total size, you're using memcpy efficiently rather than strcat which would redo the length-finding work multiple times including on the string we're building. sprintf is slowish, but not compared to system calls so that would be an option (or asprintf, a GNU extension, returns a dynamically allocated string, so if you're going for compact source, it's very convenient.)
A static char pathbuf[PATH_MAX] is another option to avoid allocating, since you don't need multiple pathnames at once. PATH_MAX is not huge on normal systems, e.g. 4096 on current Linux, so it's ok to allocate a buffer of that size, especially in a program that's using it continuously. But according to the getcwd man page, on some systems it may not even be a compile-time constant. In a large program that spends a lot of its time not running this code, having a large static buffer sitting there with "dirty" data is not great. But your program isn't that. (Linux may actually allow longer paths, as long as each /-separated component is <= 255 bytes, but PATH_MAX exists for obsolete calls like getwd.)
With a static buffer, you wouldn't need to sum lengths before you start copying, so you could use stpcpy for simple and efficient concatenation. But to still get length checking, see Linux's string_copying(7) man page for a survey of the sorry state of C's length-limited string-copy functions. Its proposed stpecpy in terms of POSIX.2008 memccpy with a stop-character of 0 might be the best bet.
Or just use relative paths since you're operating in the current directory: a filename you get from readdir works with open or fopen exactly like if ./ is prepended to it.
To work with directories other than ., I think the current best practice is to use openat (man page) which is like open but interprets paths relative to a directory specified by a dirfd(DIR*). Since you have the directory open to read entries, you have an FD you can use with openat and one of those entries without doing any path concatenation. If you strace a program that opens files, you'll see glibc's open() wrapper function actually uses openat(AT_FDCWD, ... where you used to see open(... on older systems. And as a bonus, inside the kernel the path lookup is simple and cheaper, not having to recheck /foo/bar/ on the way to baz.txt, since the dirfd refer to the directory directly. If you're recursing down subdirectories like find does, child_fd = openat(dir_fd, name, O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY) is what find uses, and you might use that in a recursive C function.
There's a corresponding unlinkat so you don't need to construct a path for that, either.
Related re: saving lstat system calls when looking for subdirectories using the d_type member of a readdir result: https://stackoverflow.com/questions/39429803/how-to-list-first-level-directories-only-in-c / https://stackoverflow.com/questions/23958040/checking-if-a-dir-entry-returned-by-readdir-is-a-directory-link-or-file-dent/29094555#29094555
FAILfor this case? See also: en.wikipedia.org/wiki/Vacuous_truth \$\endgroup\$