Makefile and main header file for a shell

Question

My main.c includes this header file. Please let me know how I can improve it. I did my best but I don't know much about code conventions in C (I come from Java and Python).

#define _XOPEN_SOURCE 500

#ifndef OPENSHELL_OPENSHELL_H
#define OPENSHELL_OPENSHELL_H
#define    CMD_LEN        1024
#define    EXPAND_ALLOC    1024
#define    PATH_LEN    1024
#define    isBlank(ch)    (((ch) == ' ') || ((ch) == '\t'))
#define    isDecimal(ch)    (((ch) >= '0') && ((ch) <= '9'))
#define    MAX_SOURCE    10
#define    isWildCard(ch)    (((ch) == '*') || ((ch) == '?') || ((ch) == '['))
#define    CHUNK_INIT_SIZE    4
#define    DEBUG TRUE
#ifndef VERSION
#define VERSION2 "v0.1a"
#endif

#include <string.h>
#include <dirent.h>
#include <utime.h>
#include <stdbool.h>
#include <getopt.h>

/*
 * A chunk of data.
 * Chunks contain data which is allocated as needed, but which is
 * not freed until all of the data needs freeing, such as at
 * the beginning of the next command.
 */
typedef struct chunk CHUNK;

struct chunk {
    CHUNK *next;
    char data[CHUNK_INIT_SIZE];    /* actually of varying length */
};
/*
 * One entry of the command table.
 */
typedef struct {
    const char *name;

    int        (*func)(int argc, const char **argv);

    int minArgs;
    int maxArgs;
    const char *description;
    const char *usage;
} CommandEntry;

typedef int Pipe[2];

static struct option options[] = {
        {"with_param", 1, 0, 'p'},
        {"version",    0, 0, 'v'},
        {"help",       0, 0, 'h'},
        {0,            0, 0, 0}
};
struct command {
    char *const *argv;
};
extern void freeChunks(void);

extern char **str_split(char *a[], char *a_str, const char a_delim);

extern int do_checkenv(int argc, const char **argv);

extern int do_cd(int argc, const char **argv);

extern int do_add2path(int argc, const char **argv);

extern int do_exit(int argc, const char **argv);

extern int do_help(int argc, const char **argv);

extern int do_kill(int argc, const char **argv);

int spawn_proc(int in, int out, struct command *cmd);

void fork_pipes(int n, struct command *cmd);

int file_exist(char *filename);

extern bool makeArgs(const char *cmd, int *argcPtr, const char ***argvPtr, bool pipe, int g, int h);

#endif
#define VERSION "v0.160429-21-g54bb-dirty"

The above file is versioned at github. You can also review the project's Makefile:

CC = gcc
GIT_VERSION := $(shell git describe --abbrev=4 --dirty --always --tags)
CFLAGS := $(CFLAGS) -L/usr/local/include/ -L/usr/include -pedantic -std=c99 -Wall -O3 -g -DVERSION=\"$(GIT_VERSION)\" -ledit -lncurses

LDIRS = -L/usr/local/lib -L/usr/lib
LIBS =su -lcurses

shell: main.o
    $(CC) -o shell main.o errors.c util.c pipeline.c -ledit -lncurses -lcurses

main.o: main.c errors.c util.c
USERNAME := $(shell whoami >> username.txt)
GIT:= $(shell head -n -1 openshell.h > temp.txt ; mv temp.txt openshell.h;git describe --abbrev=4 --dirty --always --tags > VERSION; echo "\#define VERSION \"$(GIT_VERSION)\"" >> openshell.h)

.PHONY: clean
clean:
    rm -f *.o

As seen in the Makefile, I'm writing the git version number to the header file with a shell instruction. Maybe I should do this before I compile to be able to get a "clean" version, otherwise I will always differ from a tag just by writing the new tag.

Answer 1

1. What is the header file's name? The point being that the name of the header file, like xopen.h should have some connection to the items inside. Further, the objects declares and macros defines are all over the name space. Consider some convention. Put do_...() in do.h, CommandEntry in CommandEntry.h.

2. Strange #define. Did you mean #define VERSION "v0.1a"?

   #ifndef VERSION
   //#define VERSION2 "v0.1a"
   #define VERSION "v0.1a"
   #endif
   

3. static struct option options[] will create a options[] with each *.c this include file is used. Certainly not a good idea because it is static. `static objects are best for the *.c file and not *.h header files.

4. Odd to have #define _XOPEN_SOURCE 500 and #define VERSION "v0.160429-21-g54bb-dirty" outside the include guard #ifndef OPENSHELL_OPENSHELL_H. Suggest putting them inside unless you are attempting something novel.

5. Line spacing is odd. Why no line space after a struct definition, yet spaces between similar function declarations.

   struct command {
       char *const *argv;
   };
   extern void freeChunks(void);
   
   extern char **str_split(char *a[], char *a_str, const char a_delim);
   
   extern int do_checkenv(int argc, const char **argv);
   

6. The horizontal space is odd here - perhaps an effect of using tabs?

   #define    CMD_LEN        1024
   #define    EXPAND_ALLOC    1024
   #define    PATH_LEN    1024
   #define    isBlank(ch)    (((ch) == ' ') || ((ch) == '\t'))
   #define    isDecimal(ch)    (((ch) >= '0') && ((ch) <= '9'))
   #define    MAX_SOURCE    10
   

7. All UPPERCASE identifiers are often considered macros. Code with CHUNK may confuse viewers of your code. Re:typedef struct chunk CHUNK;

8. Not using all UPPERCASE for defines like with #define isBlank(ch) is a problem as a user of that macro does not benefit from the all uppercase hint that it is a macro. That macro uses ch twice (((ch) == ' ') || ((ch) == '\t')). Consider the effect of this usage

   int ch;
   while (isBlank(ch = getchar())) { ... }
   

9. Standard suggestion. Add name and date (year) to your code.

   // 2016 Programmer 400
   

10. C has a boolean _Bool. By using <stdbool.h> , code has access to bool, true, false. Suggest true rather than TRUE in #define DEBUG TRUE.

11. Order: In general, it is cleaner to do the define first, struct, union, typedef, then global variables and lastly function declaration. This is highly a style issue though.

12. Unclear why some function have a preceding extern and others do not. With function declarations, extern is not necessary. To use or not is a style issue, but it should certainly be consistent within a *.h file.

    // to extern or not to extern, that is the question.  
    extern int do_kill(int argc, const char **argv);
    int spawn_proc(int in, int out, struct command *cmd);