Skip to content

Commit

Permalink
Remove all defines
Browse files Browse the repository at this point in the history
  • Loading branch information
xyproto committed Jan 21, 2024
1 parent 26c2006 commit b94ad5d
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 3 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
.PHONY: clean install test uninstall

CFLAGS ?= -std=c11 -O2 -pipe -fPIC -fno-plt -fstack-protector-strong -D_GNU_SOURCE -z norelro -Wall -Wextra -Wpedantic -Wfatal-errors
CFLAGS ?= -std=c2x -O2 -pipe -fPIC -fno-plt -fstack-protector-strong -D_GNU_SOURCE -z norelro -Wall -Wextra -Wpedantic -Wfatal-errors
UNAME_S := $(shell uname -s)
ifeq ($(UNAME_S),Darwin)
CFLAGS := -std=c11 -O2 -pipe -fPIC -fstack-protector-strong -Wall -Wextra -Wpedantic -Wfatal-errors
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

Tested on Arch Linux and macOS, where it builds, runs and all tests pass.

A compiler that supports C23 / `-std=c2x` is required.

### What is it, and what can it do?

`xxd` or `tinyxxd` can be used to view binary or text files as hex codes. It is often installed on UNIX-like systems. It can dump files to hex, and also do the same thing in reverse: create files from hex codes.
Expand Down
4 changes: 2 additions & 2 deletions main.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
#include <unistd.h>

// Change this if more columns should ever be needed
#define COLS 256
constexpr auto COLS = 256;

// For static declarations of buffers
#define LLEN ((2 * (int)sizeof(unsigned long)) + 4 + (9 * COLS - 1) + COLS + 2)
constexpr auto LLEN = ((2 * (int)sizeof(unsigned long)) + 4 + (9 * COLS - 1) + COLS + 2);

// HexType is the different hextypes known by this program
enum HexType {
Expand Down

3 comments on commit b94ad5d

@oliverkwebb
Copy link

@oliverkwebb oliverkwebb commented on b94ad5d Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this is beneficial.

In a modern compiler (Such as one that would support c2x), The end code is optimized so that the difference between the 2 is nonexistent, On GCC 13.2.1 (amd64), they produce the exact same executable.

I'm not saying that constexpr isn't useful, but the only thing that this patch does is make the program harder to build. (The version with the #define's actually was strictly compliment with not only c11, but c99 too!).

Simple #define macros that are essentially numbers don't usually make warnings and errors harder to track like they do in #define-ed functions. Nor does it make the code significantly uncleaner. Nor is there any attempt to take the address of these variables, and since they are redefined as constants there's no attempt to modify them.

Anyways, the program is much easier to build with older compilers without this commit. And the end result is the same for any compiler that does constant folding.

@xyproto
Copy link
Owner Author

@xyproto xyproto commented on b94ad5d Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliverkwebb

  1. I strongly dislike the way #defines and #includes work in C. Preprocessing is a hack.
  2. I am planning to port this program to ie. Zig. Removing all #defines is a natural step in that direction.
  3. ViM's xxd.c is still available for legacy compilers.

@xyproto
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliverkwebb After having tried to port the program from C to Zig, it really does not feel like an improvement. I agree that if the program should stay as C, it can just as well support C11 as well.

So, now the defines are back. I will try to port it to a more memory safe language than C, sooner or later, but for now, it's C11.

Please sign in to comment.