Request for source code review of simple Ising model

[Udyant Wig]

While browsing around the Physics Stack Exchange, I came across an
answer by Ron Maimon*, and I saw that I could implement the simulation
described therein. I first did a version in Emacs Lisp, on reviewing
which I felt that I could develop a version in C, using the Emacs Lisp
version as a rough guide.

I quote from the README:

Briefly, the Ising model is a theoretical description of
ferromagnetism, which arises when the magnetic moments of atomic
spins align in the same direction. It is used to study phase
transitions and the emergence of equilibrium configurations.

I have not gone into the physics at all. I only model the changing
configurations of a lattice of bits.

A copy of the program is at Bitbucket at this URL:
https://bitbucket.org/udyant/simplified-square-lattice-ising-model/src

I have used the implementation of linked lists from kazlib written by
Kaz Kylheku, and found at this URL:

http://www.kylheku.com/~kaz/kazlib.html

The source code is used under the BSD license mentioned within them.
The license is reproduced below:

Copyright 1996-2012
Kaz Kylheku <[email protected]>
Vancouver, Canada
All rights reserved.

BSD License:

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:

1. Redistributions of source code must retain the above copyright
notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
notice, this list of conditions and the following disclaimer in
the documentation and/or other materials provided with the
distribution.
3. The name of the author may not be used to endorse or promote
products derived from this software without specific prior
written permission.

THIS SOFTWARE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.

I would like to request the readers to go over the source and suggest
any improvements to layout, style, structure, etc. I would be highly
appreciative and very grateful.

The amalgamated source is about 628 lines; I have spaced the modules by
two empty lines.


/* common.h begins */

#ifndef COMMON_H
#define COMMON_H

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <ctype.h>
#include <string.h>
#include <errno.h>

extern double beta;
extern int dimension;
extern int scale;
extern int minority_size;

#endif

/* common.h ends */


/* common.c begins */

#include "common.h"

double beta = 0.5;
int dimension = 3;
int scale = 100;
int minority_size = 1;

/* common.c ends */


/* utilities.h begins */

#ifndef UTILITIES_H
#define UTILITIES_H

#include "common.h"

bool random_bit (void);
bool is_all_zeroes (char *string);
bool is_positive_integer (char *string);
int how_many (const char *s, int c);
int find (const char string [], char character, int offset);
char *copy_substring (char *substring, \
const char *string, \
size_t start, \
size_t end);

#endif

/* utilities.h ends */


/* utilities.c begins */

#include "utilities.h"

bool random_bit (void)
{
return rand () % 2;
}

bool is_all_zeroes (char *string)
{
char *sp;

for (sp = string; *sp != '\0'; sp++) {
if (*sp != '0') {
return false;
}
}

return true;
}

bool is_positive_integer (char *string)
{
char *sp;

for (sp = string; *sp != '\0'; sp++) {
if (!isdigit (*sp)) {
return false;
}
}

return (is_all_zeroes (string) ? false : true);
}

/* From /C: A Reference Manual/, 5th ed., by Harbison and Steele
* page 352
*/
int how_many (const char *s, int c)
{
int n = 0;
if (c == 0) return 0;
while (s) {
s = strchr (s, c);
if (s) n++, s++;
}
return n;
}

int find (const char string [], char character, int offset)
{
int length = strlen (string);
int i;

for (i = offset; i < length; i++) {
if (string == character)
return i;
}

return -1;
}

char *copy_substring (char *substring, \
const char *string, \
size_t start, \
size_t end)
{
size_t length = end - start + 1;

errno = 0;
substring = malloc (1 + length);
if (substring == NULL) {
fprintf (stderr, "copy_substring: %s\n", strerror (errno));
exit (1);
}

strncpy (substring, string + start, length - 1);
substring [length] = '\0';

return substring;
}


/* utilities.c ends */


/* bits.h begins */

#ifndef BITS_H
#define BITS_H

#include "common.h"

struct bit {
int x;
int y;
int energy;
bool bitvalue;
};

typedef struct bit bit;

/* Bits */
bool bits_equal (bit one, bit two);
bool negate (bit *b);
bit complement (bit *b);
void flip (bit *b);
void dump_bit (bit b);

/* Lattices */
bit **allocate_lattice (bit **lattice);
void free_lattice (bit **lattice);
void print_lattice (bit **lattice);
bool lattices_equal (bit **a, bit **b);

#endif

/* bits.h ends */


/* bits.c begins */

#include "bits.h"

/* Bits */
bool bits_equal (bit one, bit two)
{
return one.bitvalue == two.bitvalue;
}

bool negate (bit *b)
{
return !b->bitvalue;
}

bit complement (bit *b)
{
bit complement;

complement.x = b->x;
complement.y = b->y;
complement.energy = b->energy;
complement.bitvalue = negate (b);

return complement;
}

void flip (bit *b)
{
b->bitvalue = negate (b);
}

void dump_bit (bit b)
{
printf ("x := %d\n", b.x);
printf ("y := %d\n", b.y);
printf ("energy := %d\n", b.energy);
printf ("bitvalue := %d\n", b.bitvalue);
}


/* Lattices */
bit **allocate_lattice (bit **lattice)
{
/* bit **lattice; */
int i;

errno = 0;
lattice = malloc (dimension * sizeof (bit *));
if (lattice == NULL)
{
fprintf (stderr, "%s\n", strerror (errno));
exit (1);
}
for (i = 0; i < dimension; i++)
{
lattice = malloc (dimension * sizeof (bit));
if (lattice == NULL)
{
fprintf (stderr, "%s\n", strerror (errno));
exit (1);
}
}

return lattice;
}

void free_lattice (bit **lattice)
{
int i;

for (i = 0; i < dimension; i++) {
free (lattice );
}

free (lattice);
}

void print_lattice (bit **lattice)
{
int i, j;

for (i = 0; i < dimension; i++) {
for (j = 0; j < dimension; j++) {
printf ("%d", (lattice [j]).bitvalue);
/*
dump_bit (lattice [j]);
*/
}
putchar ('\n');
}
}

bool lattices_equal (bit **a, bit **b)
{
int i, j;
bool equal = true;

for (i = 0; i < dimension; i++) {
for (j = 0; j < dimension; j++) {
if (!bits_equal (a [j], b [j]))
equal = false;
}
}

return equal;
}

/* bits.c ends */


/* ising.h begins */

#ifndef ISING_H
#define ISING_H

#include <math.h>
#include <time.h>

#include "common.h"
#include "bits.h"
#include "utilities.h"

#include "list.h"

/* Core */
void check_and_collect_neighbor (list_t *neighborhood, int x, int y);
list_t *neighbors (list_t *neighborhood, bit b);
int energy (bit b);
void calculate_energies (void);
void ising_flip (bit *b);
void iterate (void);
void print_iteration (const char *format_string, int count);

/* Housekeeping */
static void dump_neighbors (list_t *neighbors);
static int count_ones (void);
static int count_zeroes (void);

/* Error checking */
void minority_error (void);
void beta_error (void);
void dimension_error (void);

#endif

/* ising.h ends */


/* ising.c begins */

#include "ising.h"

static bit **ising_lattice;

const char initial_format [] = "Initial configuration %6d";
const char iteration_format [] = "Iteration %6d";
const char final_format [] = "Final configuration %6d";


/* Core */
void check_and_collect_neighbor (list_t *neighborhood, int x, int y)
{
lnode_t *neighbor;
bit *payload;

if (x >= 0 && y >= 0 && x < dimension && y < dimension) {
payload = &ising_lattice [x] [y];
neighbor = lnode_create (payload);

if (neighbor == NULL) {
fprintf (stderr, "%s\n", strerror (errno));
lnode_destroy (neighbor);
free (payload);
exit (1);
}

list_append (neighborhood, neighbor);
}
}

list_t *neighbors (list_t *neighborhood, bit b)
{
neighborhood = list_create (-1);

check_and_collect_neighbor (neighborhood, b.x, b.y - 1);
check_and_collect_neighbor (neighborhood, b.x, b.y + 1);
check_and_collect_neighbor (neighborhood, b.x - 1, b.y);
check_and_collect_neighbor (neighborhood, b.x + 1, b.y);

return neighborhood;
}

int energy (bit b)
{
int e = 0;
lnode_t *neighbor;
bit *rover;
list_t *neighborhood = NULL;

neighborhood = neighbors (neighborhood, b);
for (neighbor = list_first (neighborhood); neighbor != NULL; \
neighbor = list_next (neighborhood, neighbor)) {
rover = lnode_get (neighbor);
if (b.bitvalue != rover->bitvalue) {
e++;
}
}

list_destroy_nodes (neighborhood);
list_destroy (neighborhood);

return e;
}

void calculate_energies (void)
{
int i, j;

for (i = 0; i < dimension; i++) {
for (j = 0; j < dimension; j++) {
(ising_lattice [j]).energy = energy (ising_lattice [j]);
}
}
}

void ising_flip (bit *b)
{
int old_energy = b->energy;
int new_energy = energy (complement (b));
int delta_energy = new_energy - old_energy;

if (delta_energy > 0) {
double ising_threshold = exp (-(beta * delta_energy));
double random_value = rand () % scale / (double) scale;

if (ising_threshold < random_value) {
flip (b);
}
}
else {
flip (b);
}
}

void iterate (void)
{
int x = (int) rand () % dimension;
int y = (int) rand () % dimension;

ising_flip (&ising_lattice [x] [y]);
calculate_energies ();
}

void print_iteration (const char *format_string, int count)
{
size_t i;
size_t length = strlen (format_string);

putchar ('\n');
printf (format_string, count);
putchar ('\n');
for (i = 0; i < length + 3; i++)
putchar ('-');
putchar ('\n');
print_lattice (ising_lattice);
}


/* Housekeeping */
static void dump_neighbors (list_t *neighbors)
{
bit *rover;
lnode_t *neighbor;

for (neighbor = list_first (neighbors); neighbor != NULL; \
neighbor = list_next (neighbors, neighbor)) {
rover = lnode_get (neighbor);
dump_bit (*rover);
}
}

static int count_ones (void)
{
int count = 0;
int i, j;

for (i = 0; i < dimension; i++) {
for (j = 0; j < dimension; j++) {
if ((ising_lattice [j]).bitvalue == true) {
count++;
}
}
}

return count;
}

static int count_zeroes (void)
{
int count = 0;
int i, j;

for (i = 0; i < dimension; i++) {
for (j = 0; j < dimension; j++) {
if ((ising_lattice [j]).bitvalue == false) {
count++;
}
}
}

return count;
}


/* Error checking */
void minority_error (void)
{
fputs ("minority_size <argv [3]> is not a nonnegative integer\n", stderr);
exit (2);
}

void beta_error (void)
{
fputs ("beta <argv [2]> is not positive floating-point number.\n", stderr);
exit (2);
}

void dimension_error (void)
{
fputs ("dimension <argv [1]> is not a nonnegative integer.\n", stderr);
exit (2);
}


/* Return codes:
* 0 Successful execution
* 1 Memory allocation failed
* 2 Wrong form of argument
*/
int main (int argc, char *argv [])
{
if (argc > 3) {
if (is_positive_integer (argv [3]) || is_all_zeroes (argv [3])) {
minority_size = atoi (argv [3]);
}
else {
minority_error ();
}

if (minority_size < 0) {
minority_size = 0;
}
else if (minority_size > dimension * dimension) {
minority_size = dimension * dimension;
}
else if (minority_size > (dimension * dimension) / 2) {
minority_size = (dimension * dimension) - minority_size;
}
}
if (argc > 2) {
char *beta_string = argv [2];
size_t length = strlen (beta_string);

if (how_many (beta_string, '.') == 1) {
int position_decimal = find (beta_string, '.', 0);
char *integral_part = NULL;
char *fractional_part = NULL;

integral_part = copy_substring (integral_part, \
beta_string, \
0, \
position_decimal - 1);
if (is_positive_integer (integral_part) || \
is_all_zeroes (integral_part)) {
fractional_part = copy_substring (fractional_part, \
beta_string, \
position_decimal + 1, \
length - 1);
if (is_positive_integer (fractional_part) || \
is_all_zeroes (fractional_part)) {
beta = atof (argv [2]);
}
else {
beta_error ();
}

free (fractional_part);
}
else {
beta_error ();
}

free (integral_part);
}
else {
beta_error ();
}
}
if (argc > 1) {
if (is_positive_integer (argv [1]))
dimension = atoi (argv [1]);
else {
dimension_error ();
}
}

srand (time (0));

int i, j;
bit b;

/* Allocate the square Ising lattice */
ising_lattice = allocate_lattice (ising_lattice);
for (i = 0; i < dimension; i++) {
for (j = 0; j < dimension; j++) {
b.x = i;
b.y = j;
b.energy = 0;
b.bitvalue = random_bit ();

ising_lattice [j] = b;
}
}
calculate_energies ();


/* Begin simulation */
print_iteration (initial_format, 0);

long k = 1;
while (true) {
if ((count_zeroes () == minority_size) || \
(count_ones () == minority_size)) {
break;
}
iterate ();
print_iteration (iteration_format, k++);
}
print_iteration (final_format, k);


/* End simulation */
free_lattice (ising_lattice);
exit (0);
}


/* ising.c ends */


The answer mentioned earlier:
* Ron Maimon (http://physics.stackexchange.com/users/4864/ron-maimon),
How do you start self-learning physics, URL (version: 2012-07-31):
http://physics.stackexchange.com/q/33223


Udyant Wig, programming novice

 

[Malcolm McLean]

While browsing around the Physics Stack Exchange, I came across an
answer by Ron Maimon*, and I saw that I could implement the simulation
described therein. I first did a version in Emacs Lisp, on reviewing
which I felt that I could develop a version in C, using the Emacs Lisp
version as a rough guide.

Lisp is for expressing programming ideas in a flexible, elegant way. C is for
bit bashing. Whilst you can take Lisp constructs and translate them into
C equivalents, you're not really using the language as intended.

In C, and Ising model is something like

typedef struct
{
int width;
int height;
unsigned char *mesh;
double energy;

} ISING;

To make it easy, use one byte per cell.
Set one up by calling malloc to allocate width * height bytes. Set them to
an initial value, eg 1 or 0 with a 50% chance of each. Do this by stepping
through the mesh with a for loop.
To access an element, do

model->mesh[y*model->width+x];

Nowadays compilers will generate very efficient code for this. In the bad old days you had to do tricks to prevent unnecessary multiplies. You might like to
move the y * mesh->width out of the inner loop and use an increment for the x
step to see if it makes any difference on your system.
C is all about very tight loops stepping through arrays, and doing simple
operations on them.

For an Ising model, you need special code for the boundary. So go from y= 1 to height -2, x = 1 to width -2, count the neighbouring cells, and set the second
bit (cell |= 0x02 to set or cell &= ~0x02 to clear) to indicate the next
state. Then go through the edges applying special rules for the edge
conditions. Then you cna either keep a flag indicating that bit 0 or bit
1 is current, or you can step through again and set bit 0 to bit 1 (
cell >>= 1 or cell = (cell & 0x02) ? 1 : 0; )

If you write C idiomatically and half way efficiently, it is lightening
fast. You can step a thousand by a thousand Ising model, probably in about
a millisecond.

 

[Udyant Wig]

| Lisp is for expressing programming ideas in a flexible, elegant way. C
| is for bit bashing. Whilst you can take Lisp constructs and translate
| them into C equivalents, you're not really using the language as
| intended.

I take it that the code I posted does not behoove a native speaker of
C, and that I attempted to speak with a Lisp accent. Since I did not
have experience with bit-mangling (in any language), I had not thought
of a bit-mangling solution.

| In C, and Ising model is something like
|
| typedef struct
| {
| int width;
| int height;
| unsigned char *mesh;
| double energy;
|
| } ISING;

This is one place that puzzled me. I quote from the Physics Stack
Exchange answer that I linked to:

Make a 2d grid of bits with value 0 or 1. Choose a bit at random,
and calculate the energy it feels with its neighbors, by counting
the number of neighbors that have a different bit value. The energy
is this number.

As I read it, each cell has its own energy, not the entire mesh. I
cannot see how to handle the energy of each individual cell using the
above struct.

| To make it easy, use one byte per cell. Set one up by calling malloc
| to allocate width * height bytes. Set them to an initial value, eg 1
| or 0 with a 50% chance of each. Do this by stepping through the mesh
| with a for loop. To access an element, do
|
| model->mesh[y*model->width+x];

Is faking a multidimensional array by a vector always deemed good (read
"efficient")? When should one use multidimensional arrays and when
should one fake them by vectors and manual subscripting?

| Nowadays compilers will generate very efficient code for this. In the
| bad old days you had to do tricks to prevent unnecessary
| multiplies. You might like to move the y * mesh->width out of the
| inner loop and use an increment for the x step to see if it makes any
| difference on your system.

I will look into this.

| C is all about very tight loops stepping through arrays, and doing
| simple operations on them.

A philosophical point I will remember.

| For an Ising model, you need special code for the boundary. So go from
| y= 1 to height -2, x = 1 to width -2, count the neighbouring cells,
| and set the second bit (cell |= 0x02 to set or cell &= ~0x02 to clear)
| to indicate the next state. Then go through the edges applying special
| rules for the edge conditions. Then you cna either keep a flag
| indicating that bit 0 or bit 1 is current, or you can step through
| again and set bit 0 to bit 1 ( cell >>= 1 or cell = (cell & 0x02) ? 1
| : 0; )

I found this to be clever and elegant.

| If you write C idiomatically and half way efficiently, it is
| lightening fast. You can step a thousand by a thousand Ising model,
| probably in about a millisecond.

I ran my program for some minutes with a 1000x1000 lattice, a very low
value of beta (which encourages rapid change; high values tend to
increase the "inertia" of the system), and a cutoff size of 1000, which
means that if the lattice contains either 1000 ones or 1000 zeroes out
of all the bits, then the simulation is finished. I closed it manually
because I did not see it ending any time soon.

If I can adopt some of the bit-mangling ideas, then perhaps the
situation might change.

The comments and ideas were valued very much.

Udyant Wig

 

[Malcolm McLean]

Is faking a multidimensional array by a vector always deemed good (read
"efficient")? When should one use multidimensional arrays and when
should one fake them by vectors and manual subscripting?

It's a deficiency in the language. It doesn't have good facilities for multi-dimensional arrays. That was largely alleviated in C99, but the standard wasn't widely implemented.
If you don't know array dimensions at compile time, or if you have to pass the array to subroutines which are best written as taking an variable mxn array rather than a fixed-size one, then it's
easier to allocate a 1D array and do the calculations explicitly.

 

[Udyant Wig]

Over the past few days, I have revised and rewritten the program to
use a vector of unsigned characters (i.e., an 8-bit byte) for the data
representation of a square-lattice Ising model:

7 6 5 4 3 2 1 0
*--*--*--*--*--*--*--*--*
|X |X |X |E |E |E |NS|CS|
*__*__*__*__*__*__*__*__*

CS := bitvalue of this cell in the current state
NS := bitvalue of this cell in the next state
E := binary representation of energy: 0 to 4
X := unused

Testing shows that this version is a bit faster than the one I posted
earlier. It is also shorter in terms of code. But it still takes
quite a long time for 10x10 lattices.

The source and Makefile follow:


/* common.h begins */

#ifndef COMMON_H
#define COMMON_H

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <math.h>
#include <stdbool.h>
#include <time.h>
#include <ctype.h>

#endif

/* common.h ends */


/* utilities.h begins */

#ifndef UTILITIES_H
#define UTILITIES_H

#include "common.h"

bool is_all_zeroes (char *string);
bool is_positive_integer (char *string);
int how_many (const char *s, int c);
int find (const char string [], char character, int offset);
char *copy_substring (char *substring, const char *string, size_t start, size_t end);

#endif

/* utilities.h ends */


/* utilities.c begins */

#include "utilities.h"

bool is_all_zeroes (char *string)
{
char *sp;

for (sp = string; *sp != '\0'; sp++) {
if (*sp != '0') {
return false;
}
}

return true;
}

bool is_positive_integer (char *string)
{
char *sp;

for (sp = string; *sp != '\0'; sp++) {
if (!isdigit (*sp)) {
return false;
}
}

return (is_all_zeroes (string) ? false : true);
}

/* From /C: A Reference Manual/, 5th ed., by Harbison and Steele
* page 352
*/
int how_many (const char *s, int c)
{
int n = 0;
if (c == 0) return 0;
while (s) {
s = strchr (s, c);
if (s) n++, s++;
}
return n;
}

int find (const char string [], char character, int offset)
{
int length = strlen (string);
int i;

for (i = offset; i < length; i++) {
if (string == character)
return i;
}

return -1;
}

char *copy_substring (char *substring, const char *string, size_t start, size_t end)
{
size_t length = end - start + 1;

errno = 0;
substring = malloc (1 + length);
if (substring == NULL) {
fprintf (stderr, "copy_substring: %s\n", strerror (errno));
exit (1);
}

strncpy (substring, string + start, length - 1);
substring [length] = '\0';

return substring;
}

/* utilities.c ends */


/* bitsing.h begins */

#ifndef BITSING_H
#define BITSING_H

typedef unsigned char byte;

/* Lattice */
byte *allocate_lattice (byte *lattice);
void initialize_lattice (void);
void print_lattice (void);

/* Core */
int check_and_add_neighbor (byte cell, int col, int row);
void next_state (void);

/* Miscellaneous */
int count_ones (void);
void print_iteration (const char *format_string, int count);

/* Error checking */
void minority_error (void);
void beta_error (void);
void dimension_error (void);

#endif

/* bitsing.h ends */


/* bitsing.c begins */

#include "common.h"
#include "bitsing.h"
#include "utilities.h"

#define ENERGY_MASK 0x1c

static int dimension = 3;
static double beta = 0.5;
static int minority_size = 1;

static byte *lattice;


/* Lattice */
byte *allocate_lattice (byte *lattice)
{
errno = 0;
lattice = malloc (dimension * dimension * sizeof *lattice);
if (lattice == NULL) {
fprintf (stderr, "allocate_lattice: %s\n", strerror (errno));
exit (1);
}

return lattice;
}

void initialize_lattice (void)
{
int row, col;
byte *cell;

for (col = 0; col < dimension; col++) {
for (row = 0; row < dimension; row++) {
cell = &lattice [col * dimension + row];
/* Clear second bit */
*cell &= ~0x02;
/* Set second bit randomly */
*cell |= ((byte) (rand () % 2)) << 1;
}
}
}

void print_lattice (void)
{
int row, col;

for (col = 0; col < dimension; col++) {
for (row = 0; row < dimension; row++) {
printf ("%d", lattice [col * dimension + row] & 0x01);
}
putchar ('\n');
}
}


/* Core */
int check_and_add_neighbor (byte cell, int col, int row)
{
byte neighbor;
if (0 <= col && 0 <= row && col < dimension && row < dimension) {
neighbor = lattice [col * dimension + row];
return ((cell & 0x02) ^ (neighbor & 0x02));
}
return 0;
}

void next_state (void)
{
int row, col;
byte *cell;

for (col = 0; col < dimension; col++) {
for (row = 0; row < dimension; row++) {
cell = &lattice [col * dimension + row];
*cell |= (0x01 & ((*cell & 0x02) >> 1));
}
}
}


/* Miscellaneous */
int count_ones (void)
{
int count = 0;
int row, col;

for (col = 0; col < dimension; col++) {
for (row = 0; row < dimension; row++) {
if ((lattice [col * dimension + row] & 0x01) == (byte) 1) {
count++;
}
}
}

return count;
}

static const char initial_format [] = \
"Initial configuration %6d\n----------------------------\n";
static const char iteration_format [] = \
"Iteration %6d\n----------------\n";
static const char final_format [] = \
"Final configuration %6d\n--------------------------\n";

void print_iteration (const char *format_string, int count)
{
putchar ('\n');
printf (format_string, count);
print_lattice ();
}


/* Error checking */
void minority_error (void)
{
fputs ("minority_size <argv [3]> is not a nonnegative integer\n", stderr);
exit (2);
}

void beta_error (void)
{
fputs ("beta <argv [2]> is not positive floating-point number.\n", stderr);
exit (2);
}

void dimension_error (void)
{
fputs ("dimension <argv [1]> is not a nonnegative integer.\n", stderr);
exit (2);
}


/* Return codes:
* 0 -- success
* 1 -- memory allocation failure
* 2 -- invalid command line argument
*/
int main (int argc, char *argv [])
{
if (argc > 3) {
if (is_positive_integer (argv [3]) || is_all_zeroes (argv [3])) {
minority_size = atoi (argv [3]);
}
else {
minority_error ();
}

if (minority_size < 0) {
minority_size = 0;
}
else if (minority_size > dimension * dimension) {
minority_size = dimension * dimension;
}
else if (minority_size > (dimension * dimension) / 2) {
minority_size = (dimension * dimension) - minority_size;
}
}
if (argc > 2) {
char *beta_string = argv [2];
size_t length = strlen (beta_string);

if (how_many (beta_string, '.') == 1) {
int position_decimal = find (beta_string, '.', 0);
char *integral_part = NULL;
char *fractional_part = NULL;

integral_part = copy_substring (integral_part, \
beta_string, \
0, \
position_decimal - 1);
if (is_positive_integer (integral_part) || \
is_all_zeroes (integral_part)) {
fractional_part = copy_substring (fractional_part, \
beta_string, \
position_decimal + 1, \
length - 1);
if (is_positive_integer (fractional_part) || \
is_all_zeroes (fractional_part)) {
beta = atof (argv [2]);
}
else {
beta_error ();
}

free (fractional_part);
}
else {
beta_error ();
}

free (integral_part);
}
else {
beta_error ();
}
}
if (argc > 1) {
if (is_positive_integer (argv [1]))
dimension = atoi (argv [1]);
else {
dimension_error ();
}
}

srand (time (0));

lattice = allocate_lattice (lattice);
initialize_lattice ();
next_state ();
print_iteration (initial_format, 0);

long count = 1;
while (true) {
int rrow, rcol;
byte rcell, ccell;
int old_energy;
int new_energy;
int delta;

rrow = rand () % dimension;
rcol = rand () % dimension;
rcell = lattice [rcol * dimension + rrow];
ccell = (rcell ^ 0x02) & 0x02; /* comlemented cell */
old_energy = (rcell & ENERGY_MASK) >> 2;

new_energy = check_and_add_neighbor (ccell, rrow - 1, rcol);
new_energy = check_and_add_neighbor (ccell, rrow + 1, rcol);
new_energy = check_and_add_neighbor (ccell, rrow, rcol - 1);
new_energy = check_and_add_neighbor (ccell, rrow, rcol + 1);

delta = new_energy - old_energy;
if (delta < 0) {
lattice [rcol * dimension + rrow] = ccell;
/* clear energy */
ccell &= ~ENERGY_MASK;
/* set energy */
ccell |= (ENERGY_MASK & ((byte) new_energy << 2));
}
else {
double ising_window = exp (-(beta * delta));
double random_value = (double) (rand () % 100) / 100.0;

if (random_value < ising_window) {
lattice [rcol * dimension + rrow] = ccell;
/* clear energy */
ccell &= ~ENERGY_MASK;
/* set energy */
ccell |= (ENERGY_MASK & ((byte) new_energy << 2));
}
}

print_iteration (iteration_format, count++);
int ones_count = count_ones ();
if ((ones_count == minority_size) || \
((dimension * dimension - ones_count) == minority_size)) {
break;
}

next_state ();
}

print_iteration (final_format, count);
free (lattice);
exit (0);
}

/* bitsing.c ends */


# Makefile begins

CC = gcc
CFLAGS = -O0 -g -std=c99 -pedantic -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes

bitsing: bitsing.o bitsing.h common.h utilities.o utilities.h
$(CC) $(CFLAGS) -o bitsing bitsing.o utilities.o -lm

depend: .depend
$(CC) $(CFLAGS) -E -MM *.c > .depend

clean:
rm *.o bitsing

# Makefile ends


Udyant Wig

 

[Ben Bacarisse]

The source and Makefile follow:

There are a few things I find stylistically odd, but there is a more
significant issue that you should address: malloc does not initialise
the data it allocates. Since your "initialize_lattice" function just
sets the cells based on their current content, at no time is the data
made determinate. Obviously, you usually get zeros (your system may
even guarantee you get zeros) but C does not guarantee this.

Stylistically, I'd make more use of function arguments. I'd make the
lattice an argument of the functions that work on it (the one place
where you do pass it, you don't make any use of it).

In several places (two where it might matter for speed) you could do
away with the nested loops and the index arithmetic. Given your data,
this pattern:

for (col = 0; col < dimension; col++) {
for (row = 0; row < dimension; row++) {
... lattice[col * dimension + row]...
}
}

can often be replaced with a single loop. You might even use a pointer:

byte *bp = lattice, *end = bp + dimension * dimension;
while (bp < end) {
... *bp++ ...
}

Of course, the optimiser might do most of this anyway, so there may be
no gain at all.

You use a lot of functions (good) but despite having one for each
separate command-line error condition (I wouldn't, myself) you've put
all the main work into a giant loop in main.

Finally, a trick the often helps this sort of code is to over allocate
the array to leave a border of zero cells. This means you can eliminate
the test for 'col' and 'row' being in bounds and sometimes this pays
dividends.

 

[Ike Naar]

bool is_all_zeroes (char *string)
{
char *sp;

for (sp = string; *sp != '\0'; sp++) {
if (*sp != '0') {
return false;
}
}

return true;
}

For operations like these, the strspn function comes in handy:

bool is_all_zeroes(char const *string)
{
return string[strspn(string,"0")] == '\0';
}

 

[Udyant Wig]

| There are a few things I find stylistically odd, but there is a more
| significant issue that you should address: malloc does not initialise
| the data it allocates. Since your "initialize_lattice" function just
| sets the cells based on their current content, at no time is the data
| made determinate. Obviously, you usually get zeros (your system may
| even guarantee you get zeros) but C does not guarantee this.

Should I have used calloc() or memset()?

I did run the program through valgrind and it did report errors
pertaining to uninitialized memory, but I thought that as I was never
actually using or reading from the freshly allocated memory (it was set
immediately afterwards in main()), all was well. I could be mistaken.

| Stylistically, I'd make more use of function arguments. I'd make the
| lattice an argument of the functions that work on it (the one place
| where you do pass it, you don't make any use of it).

The only function where I pass it is allocate_lattice(), which takes as
actual argument the global lattice pointer, calls malloc(), and returns
it. Should I have done the allocation in main() itself, omitting a
separate allocator?

I am still unclear as to the more general situation: given a global
pointer, should its allocation be isolated in a function, and if so,
what should it return? Also, should whichever functions operating on
the global take it as an argument or have at it directly?

| In several places (two where it might matter for speed) you could do
| away with the nested loops and the index arithmetic. Given your data,
| this pattern:
|
| for (col = 0; col < dimension; col++) {
| for (row = 0; row < dimension; row++) {
| ... lattice[col * dimension + row]...
| }
| }
|
| can often be replaced with a single loop. You might even use a pointer:
|
| byte *bp = lattice, *end = bp + dimension * dimension;
| while (bp < end) {
| ... *bp++ ...
| }
|
| Of course, the optimiser might do most of this anyway, so there may be
| no gain at all.

I replaced every set of nested loops (except the one in
print_lattice(); I have yet to figure out the calculations to print a
1D array onscreen as 2D) by the pointer loop, and I did see some
quickening. (I seeded the random number generator with an integer for
the testing runs.)

| You use a lot of functions (good) but despite having one for each
| separate command-line error condition (I wouldn't, myself) you've put
| all the main work into a giant loop in main.

Indeed. I have now found that that giant loop can be shipped off into
a new function, and said loop in main() can be replaced by a single
function call.

Why would you not have separate functions for command-line error
conditions?

| Finally, a trick the often helps this sort of code is to over allocate
| the array to leave a border of zero cells. This means you can eliminate
| the test for 'col' and 'row' being in bounds and sometimes this pays
| dividends.

I tried doing that. If the desired dimension was to be d, I had it
allocate a (d + 2)*(d + 2) lattice. I also figured out the starting
and the ending positions of the actual lattice (which was a sublattice
of the one allocated), but I found that the pointer loop I had
substituted for nested loops would have to be changed so that the
pointer roved only within the sublattice.

Determining whether the benefits of having a zero-border and no
boundary-checking four times for each randomly selected cell outweigh
the cost of the calculations necessary to rove only in the sublattice
is going to take quite a few profiled runs.


Thank you for the feedback and commentary. I am grateful. I learned
much.

Udyant Wig

 

[Malcolm McLean]

I am still unclear as to the more general situation: given a global
pointer, should its allocation be isolated in a function, and if so,
what should it return? Also, should whichever functions operating on
the global take it as an argument or have at it directly?

If caller has access to the global pointer, subroutines which operate on it should take the global as
a parameter.
So how can caller not have access to the global, given that by definition a global is accessible from
anywhere? The answer is that often you want to hide the global from caller. e.g. a cursor is inherently
global, but often you want a function getcursorxy(int *x, int *y) which functions can just call, without
bothering about the cursor implementation details.
However getcursorxy should be written like this

SCREEN *global_screen_setup;
void getcursoxy(int *x, int *y)
{
getscreencursorxy(global_screen_setup, x, y);
}

void getscreencursorx(SCREEN *screen, int *x, int *y)
{
*x = screen->cursor_x;
*y = screen->cursor_y;
}

You just want a few globals in your program, and you want to access them through a few carefully-
controlled access points. Then the program is less likely to get into a confusing and hard-to-debug
state.

 

[James Kuyper]

| There are a few things I find stylistically odd, but there is a more
| significant issue that you should address: malloc does not initialise
| the data it allocates. Since your "initialize_lattice" function just
| sets the cells based on their current content, at no time is the data
| made determinate. Obviously, you usually get zeros (your system may
| even guarantee you get zeros) but C does not guarantee this.

Should I have used calloc() or memset()?

calloc() is functionally equivalent to malloc() followed by memset().
There's no plausible reasons why calloc() would ever be significantly
slower than malloc()+memset(). However, on some systems, dynamically
allocated memory is automatically zeroed out, whether allocated by
calloc() or malloc() - on such systems, malloc()+memset() would be a
waste of time, so you should simply call calloc().

I did run the program through valgrind and it did report errors
pertaining to uninitialized memory, but I thought that as I was never
actually using or reading from the freshly allocated memory (it was set
immediately afterwards in main()), all was well. I could be mistaken.

You are. main() doesn't do anything between calling allocate_lattice()
and initialize_lattice(). allocate_lattice() does nothing to initialize
the lattice (which make sense). initialize_lattice() doesn't do anything
before reaching the following line, to initialize *cell:

*cell &= ~0x02;

Since that line is equivalent to

*cell = *cell & ~0x02;

the second *cell reads the uninitialized memory allocated by
allocate_lattice().

| Stylistically, I'd make more use of function arguments. I'd make the
| lattice an argument of the functions that work on it (the one place
| where you do pass it, you don't make any use of it).

The only function where I pass it is allocate_lattice(), which takes as
actual argument the global lattice pointer, calls malloc(), and returns
it. Should I have done the allocation in main() itself, omitting a
separate allocator?

I am still unclear as to the more general situation: given a global
pointer, should its allocation be isolated in a function, and if so,
what should it return? Also, should whichever functions operating on
the global take it as an argument or have at it directly?

As a general rule, you should avoid globals - they make it harder to
keep track of which function is responsible for each change to the
global. I'd create the pointer at block scope in main(), and pass it to
all of the functions that need access for it. In a small program, it
doesn't make much difference. However, in a larger program passing
around the pointer explicitly makes it much easier to track down which
function is responsible for something happening.

There's another advantage: if at some future time you want to create a
modified version of your program that involves running two different
Ising models at the same time, with different parameters, in order to
compare the results, using pointers makes that easy. Using globals would
not.

 

[Ben Bacarisse]

| There are a few things I find stylistically odd, but there is a more
| significant issue that you should address: malloc does not initialise
| the data it allocates. Since your "initialize_lattice" function just
| sets the cells based on their current content, at no time is the data
| made determinate. Obviously, you usually get zeros (your system may
| even guarantee you get zeros) but C does not guarantee this.

Should I have used calloc() or memset()?

There's no need because you go and set every cell to some initial value.
Calling calloc might have zero extra cost, but it's unnecessary, and
calling memset is just a waste when you subsequently set the cells
yourself.

I did run the program through valgrind and it did report errors
pertaining to uninitialized memory, but I thought that as I was never
actually using or reading from the freshly allocated memory (it was set
immediately afterwards in main()), all was well. I could be
mistaken.

You set the cells using |= and &= both of which modify an existing
indeterminate value. You do end up explicitly setting the second bit
this way, and if you never used any other bits, the code would be
correct*, but since you do use other bits, there is a problem.

It's good that you are using valgrind. It's a great tool, but you need
to be very sure before deciding that it's mistaken (though it sometimes
is).

* There's a technicality here that depends on the type used for the
cell and the possibility of trap representations but it's never wise to
use indeterminate data, even if the result happens to be determinate,
and in this case it applies to only one bit in the cell.

| Stylistically, I'd make more use of function arguments. I'd make the
| lattice an argument of the functions that work on it (the one place
| where you do pass it, you don't make any use of it).

The only function where I pass it is allocate_lattice(), which takes as
actual argument the global lattice pointer, calls malloc(), and returns
it. Should I have done the allocation in main() itself, omitting a
separate allocator?

No, my point is that the argument is useless. The function immediately
overwrites it, so there is no point in passing it at all. This is
logical -- the allocator does not need to know this pointer unless it is
going to be, one day, a re-allocator used to make new lattices from old
ones.

I am still unclear as to the more general situation: given a global
pointer, should its allocation be isolated in a function, and if so,
what should it return?

I would have, as you have, a separate allocator, and it would return, as
yours does, a pointer to the lattice it has just allocated. The only
difference is that I'd pass the size as an argument and I would not pass
the value of the lattice pointer, since it's not used.

Also, should whichever functions operating on
the global take it as an argument or have at it directly?

I'd always use a parameter. I'd aim to have all functions passed
everything they need to do their job.

| In several places (two where it might matter for speed) you could do
| away with the nested loops and the index arithmetic. Given your data,
| this pattern:
|
| for (col = 0; col < dimension; col++) {
| for (row = 0; row < dimension; row++) {
| ... lattice[col * dimension + row]...
| }
| }
|
| can often be replaced with a single loop. You might even use a pointer:
|
| byte *bp = lattice, *end = bp + dimension * dimension;
| while (bp < end) {
| ... *bp++ ...
| }
|
| Of course, the optimiser might do most of this anyway, so there may be
| no gain at all.

I replaced every set of nested loops (except the one in
print_lattice(); I have yet to figure out the calculations to print a
1D array onscreen as 2D) by the pointer loop, and I did see some
quickening. (I seeded the random number generator with an integer for
the testing runs.)

Your makefile has -O0 so you may be seeing an improvement in what it bad
generated code to start with. It maybe that gcc can make your nested
loops as fast as a single one if you give it some rope.

You are right about print_lattice. Much better to keep the 2D nature of
the lattice clear, but if you want to play, the key steps to doing with
pointers is that bp - lattice is an integer value telling you have far
into the array you are, and (bp - lattice) % dimension tells you where
you are in a row. There is some fiddling to get the details right, but
do it just for fun -- all this will do is make the function more
obscure.

| You use a lot of functions (good) but despite having one for each
| separate command-line error condition (I wouldn't, myself) you've put
| all the main work into a giant loop in main.

Indeed. I have now found that that giant loop can be shipped off into
a new function, and said loop in main() can be replaced by a single
function call.

Why would you not have separate functions for command-line error
conditions?

I just don't think it pays off in clarity. You end up with the exit
values spread about, and the message far away from the condition that
triggers it. I think

if (something_wrong) {
fputs("explain what's wrong\n", stderr);
return 2;
}

is simpler and clearer than

if (something_wrong) {
something_wrong_function();
}

with

void something_wrong_function(void)
{
fputs("explain what's wrong\n", stderr);
exit(2);
}

being somewhere else in the file. Also, you've built an obstacle to
printing more helpful errors because anything extra that's needed now has
to passed as a parameter to the error-printing function.

| Finally, a trick the often helps this sort of code is to over allocate
| the array to leave a border of zero cells. This means you can eliminate
| the test for 'col' and 'row' being in bounds and sometimes this pays
| dividends.

I tried doing that. If the desired dimension was to be d, I had it
allocate a (d + 2)*(d + 2) lattice. I also figured out the starting
and the ending positions of the actual lattice (which was a sublattice
of the one allocated), but I found that the pointer loop I had
substituted for nested loops would have to be changed so that the
pointer roved only within the sublattice.

Yes, it does not combine well with switching to a 1D loop.

Determining whether the benefits of having a zero-border and no
boundary-checking four times for each randomly selected cell outweigh
the cost of the calculations necessary to rove only in the sublattice
is going to take quite a few profiled runs.

Absolutely. Write for clarity first, and do these alterations only if
it really makes a difference.

 

[Keith Thompson]

calloc() is functionally equivalent to malloc() followed by memset().
There's no plausible reasons why calloc() would ever be significantly
slower than malloc()+memset(). However, on some systems, dynamically
allocated memory is automatically zeroed out, whether allocated by
calloc() or malloc() - on such systems, malloc()+memset() would be a
waste of time, so you should simply call calloc().

[...]

Either calloc() or memset() can be used to initialize the block of
memory to all-bits-zero, but that's not necessarily as useful as you
might expect. There's no guarantee in the language that setting a
floating-point or pointer object to all-bits-zero will set it to 0.0 or
NULL, respectively (though it very commonly will).

If you don't mind making your code a bit non-portable (and in fact
systems where 0.0 and NULL *aren't* represented as all-bits-zero are
rare), then calloc() or memset() can be reasonable. It will at least
make the initial contents of the allocated memory consistent, even if
it's not necessarily correct.

An alternative is to carefully write your code so that it never accesses
memory before it's been explicitly initialized to some meaningful value.
(Valgrind can help with that.)

 

[James Kuyper]

On 04/16/2014 12:07 PM, Keith Thompson wrote:
....

Either calloc() or memset() can be used to initialize the block of
memory to all-bits-zero, but that's not necessarily as useful as you
might expect. There's no guarantee in the language that setting a
floating-point or pointer object to all-bits-zero will set it to 0.0 or
NULL, respectively (though it very commonly will).

The code in question only uses integers, so I didn't think it worthwhile
to bring up that issue.

 

[Stefan Ram]

I'd aim to have all functions passed
everything they need to do their job.

#include <hypothetical.h>

int main( STDLIB * stdlib ){ stdlib->puts( "hello, world" ); }

 

[Keith Thompson]

On 04/16/2014 12:07 PM, Keith Thompson wrote:
...

The code in question only uses integers, so I didn't think it worthwhile
to bring up that issue.

In that case you're right. (I didn't take the time to read the code.)

Then again, the code could change, and/or it might be useful to someone
else.

 

[Kaz Kylheku]

In that case you're right. (I didn't take the time to read the code.)

Then again, the code could change, and/or it might be useful to someone
else.

Yes, somenoe else hacking away in the basement of the Smithsonian on some
machine with not-all-zero-bits null pointers, funded by a "cultural grant",
rather than actual customers or investors.

 

[Malcolm McLean]

#include <hypothetical.h>

int main( STDLIB * stdlib ){ stdlib->puts( "hello, world" ); }

Most IO systems are like that.
Typically there's some connection to the GUI which is then passed about,
like the X Display * or the windows HINSTANCE. Even stdlib only has three
global streams.

 

[Udyant Wig]

| calloc() is functionally equivalent to malloc() followed by memset().
| There's no plausible reasons why calloc() would ever be significantly
| slower than malloc()+memset(). However, on some systems, dynamically
| allocated memory is automatically zeroed out, whether allocated by
| calloc() or malloc() - on such systems, malloc()+memset() would be a
| waste of time, so you should simply call calloc().

I tried both malloc() + memset() and calloc(). I did not notice much
of a difference in running time.

| You are. main() doesn't do anything between calling allocate_lattice()
| and initialize_lattice(). allocate_lattice() does nothing to
| initialize the lattice (which make sense). initialize_lattice()
| doesn't do anything before reaching the following line, to initialize
| *cell:
|
| *cell &= ~0x02;
|
| Since that line is equivalent to
|
| *cell = *cell & ~0x02;
|
| the second *cell reads the uninitialized memory allocated by
| allocate_lattice().

Oh dear. A moment's oversight that did not yield dire results,
thankfully.

The lattice has since been zeroed after allocation via memset().

| As a general rule, you should avoid globals - they make it harder to
| keep track of which function is responsible for each change to the
| global. I'd create the pointer at block scope in main(), and pass it
| to all of the functions that need access for it. In a small program,
| it doesn't make much difference. However, in a larger program passing
| around the pointer explicitly makes it much easier to track down which
| function is responsible for something happening.

After scrutinizing the source, it turns out that I did not need the
lattice to be global at all. It is now declared in main() with no ill
effect.

| There's another advantage: if at some future time you want to create a
| modified version of your program that involves running two different
| Ising models at the same time, with different parameters, in order to
| compare the results, using pointers makes that easy. Using globals
| would not.

Ah. Another feature I did not think of.

 

[Udyant Wig]

| Either calloc() or memset() can be used to initialize the block of
| memory to all-bits-zero, but that's not necessarily as useful as you
| might expect. There's no guarantee in the language that setting a
| floating-point or pointer object to all-bits-zero will set it to 0.0 or
| NULL, respectively (though it very commonly will).

I see now that this is indeed mentioned in the discussion regarding
calloc() on page 408 of the 5th edition of Harbison and Steele:

Note that memory cleared bitwise to zero might not have the same
representation as a floating-point zero or a null pointer.

| If you don't mind making your code a bit non-portable (and in fact
| systems where 0.0 and NULL *aren't* represented as all-bits-zero are
| rare), then calloc() or memset() can be reasonable. It will at least
| make the initial contents of the allocated memory consistent, even if
| it's not necessarily correct.

As I did not use floats or doubles, I suppose the program is safe.

| An alternative is to carefully write your code so that it never accesses
| memory before it's been explicitly initialized to some meaningful value.
| (Valgrind can help with that.)

I shall keep that in mind as an alternate strategy when memory cannot
be initialized for some reason.

Running my program through Valgrind now reports no errors.

 

[Udyant Wig]

|
| There's no need because you go and set every cell to some initial
| value. Calling calloc might have zero extra cost, but it's
| unnecessary, and calling memset is just a waste when you subsequently
| set the cells yourself.

I have erred on the side of caution in the latest revision and
explicitly initialized the lattice memory with memset(). At the very
least, it made Valgrind report no errors.

| You set the cells using |= and &= both of which modify an existing
| indeterminate value. You do end up explicitly setting the second bit
| this way, and if you never used any other bits, the code would be
| correct*, but since you do use other bits, there is a problem.
|
| It's good that you are using valgrind. It's a great tool, but you
| need to be very sure before deciding that it's mistaken (though it
| sometimes is).
|
| * There's a technicality here that depends on the type used for the
| cell and the possibility of trap representations but it's never wise
| to use indeterminate data, even if the result happens to be
| determinate, and in this case it applies to only one bit in the cell.

I was doing something very dubious. I have (hopefully) set things
right by initializing the memory.

| I would have, as you have, a separate allocator, and it would return,
| as yours does, a pointer to the lattice it has just allocated. The
| only difference is that I'd pass the size as an argument and I would
| not pass the value of the lattice pointer, since it's not used.

I have done what you suggest:

byte *allocate_lattice (size_t size)
{
byte *lattice;

errno = 0;
lattice = malloc (size);
if (lattice == NULL) {
fprintf (stderr, "allocate_lattice: %s\n", strerror (errno));
exit (1);
}

memset (lattice, 0, size);
return lattice;
}

| I'd always use a parameter. I'd aim to have all functions passed
| everything they need to do their job.

That makes sense.

| Your makefile has -O0 so you may be seeing an improvement in what it
| bad generated code to start with. It maybe that gcc can make your
| nested loops as fast as a single one if you give it some rope.

I put -O0 chiefly so that the -g and -pg flags could provide
information useful to gdb and gprof.

| You are right about print_lattice. Much better to keep the 2D nature
| of the lattice clear, but if you want to play, the key steps to doing
| with pointers is that bp - lattice is an integer value telling you
| have far into the array you are, and (bp - lattice) % dimension tells
| you where you are in a row. There is some fiddling to get the details
| right, but do it just for fun -- all this will do is make the function
| more obscure.

I will try doing this as an exercise for later as it looks a touch
involved.

As an aside, profiling with gprof shows that print_lattice() takes the
lion's share of the running time. A 6x6 lattice ususally finishes
within half a minute. A 7x7 lattice takes several minutes to reach its
final configuration, even if I redirect output to /dev/null.

I had left a 10x10 lattice running yesterday before dinner (at about
9:00 in the night) and had forgotten about it. Some time after
midnight, when I chanced upon the terminal, it was still running. I
did not have the heart to see it continue.

| I just don't think it pays off in clarity. You end up with the exit
| values spread about, and the message far away from the condition that
| triggers it. I think
|
| if (something_wrong) {
| fputs("explain what's wrong\n", stderr);
| return 2;
| }
|
| is simpler and clearer than
|
| if (something_wrong) {
| something_wrong_function();
| }
|
| with
|
| void something_wrong_function(void)
| {
| fputs("explain what's wrong\n", stderr);
| exit(2);
| }
|
| being somewhere else in the file. Also, you've built an obstacle to
| printing more helpful errors because anything extra that's needed now
| has to passed as a parameter to the error-printing function.

What if the same error-checking code has to be repeated at many places?
In my case, there are four places in main() where a beta_error()
occurs. Should they be replaced by the function body? What if the
error message or something else had to be changed or something added?

One thing that is conceivable is to store all error-related functions
in separate source file. But is that a good idea?

| Absolutely. Write for clarity first, and do these alterations only if
| it really makes a difference.

As the oft-beaten Knuthian horse says: Premature optimization is
*neigh* root of all evil.

Well, that was bad.