7
\$\begingroup\$

Historical Friday: this was my first useful C program, written around 1993-1994 (with only occasional access to a compiler and runtime environment). It's also my first non-trivial Postscript, too.

Background

I was a student at the time I wrote this, and unable to transport a turntable to and from my accommodation, so I recorded my favourite albums to cassette tape. But I was already showing a perfectionist streak - hand-written box labels aren't neat enough, and the interactive graphics editors were never going to give consistent results (additionally, I'm too lazy for all the manual work required). So I needed some automation.

Input format

Input is line-oriented, in ISO 8859-1 encoding. The first line specifies the artist and the second the album name. Then there's a blank line, then a line for each track. The track listing is terminated by a line with a single . (to end a side) or with .. (to end the tape). We start again with artist and title, but they may be omitted if the same as the previous side.

Here's an example input file (this one is particularly difficult due to the long titles):

Altan
Island Angel

Tommy Peoples; The Windmill; Fintan McManus's
Bríd Óg Ní Mháille
Fermanagh Highland; Donegal Highland; John Doherty's; King George IV
An Mhaighdean Mhara
Andy de Jarlis; Ingenish; Mrs. McGhee
Humours of Andytown; Kylebrach Rambler; The Gladstone
Dúlamán
Mazurka
The Jug of Punch
Glory Reel; The Heathery Cruach
An Cailín Gaelach
Drumnagarry Strathspey; Pirrie Wirrie; Big John's Reel
Aingeal an Oileáin (Island Angel)
.

Harvest Storm

Pretty Peg; New Ships a-Sailing; The Bird's Nest; The Man From Bundoran
Dónal agus Mórag
King of the Pipers
Séamus O'Shanahan's; Walking In Liffey Street
Mo Choill
The Snowy Path
Drowsy Maggie; Rakish Paddy; Harvest Storm
'Sí do Mhaimeo I
McFarley's;  Mill na Maidi
Rosses Highlands
A Nobleman's Wedding
Bog an Lochain; Margaree Reel; The Humours of Westport
Dublin's Flowery Vale

Output

Output is a Postscript file ready for printing, with 2 to 4 labels fitting on an A4 sheet. In particular, the artist and album are consistently arranged on the spine and rear slip (with a shared name spanning both halves; otherwise divided in two). If all titles could fit on the front of the box, then we have a simple layout, taking ¼ page; otherwise we get a extended label that takes ½ page and is folded with the excess on the inside of the box.

Because we have a mix of ½-page and ¼-page outputs, I maintain 2 pages worth of "slots" into which these can be placed, so as to get the best use of paper.

I avoid widowed text by squashing to fit if it's only slightly over height of one side. Word wrapping is evaded by shrinking to fit (even if it makes for unreadable text).

sample output

The code

Although I'm very tempted to edit it, this is the code I wrote a quarter-century or so since (so the standard is C89) with no cleanup at all.

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


char *progname;
static const char * const ps_preamble[10]; /*defined after main() */

typedef struct strlist
{
  char *str;
  struct strlist *next;
} Strlist;

typedef struct tape
{
  int wide; /* true if continuation pages */
  char *artist[2]; /* 2nd may be NULL for 1-sided tape */
  char *title[2];  /* ditto */
  Strlist *songs[2];
  int nsongs[2];
} Tape;

Strlist *followlist(Strlist *s, int n)
{
  for (; s && n; s=s->next, n--)
    ;
  return s;
}

char *getline(FILE * const fp)
{
  /* Returns one line of text (any length) from fp, stripping leading
     and trailing whitespace. Returned pointer is to a region of
     malloc'ed memory, and must be free'd after use.
     Returns NUll on failure, with errno set. */

  char *line=NULL;
  const int bufinc=20;
  size_t size=0;
  int len=0;
  int c;

  while (isspace(c=fgetc(fp)) && (c!='\n'))
    ;
  if (c==EOF) return NULL;
  ungetc (c, fp);
  do {
    size += bufinc;
    line = (char *)realloc(line, size);
    if (!line) return NULL;
    if (!fgets(line+len, size-len, fp))
      { free(line); return NULL; }
    len = strlen(line);
  } while (line[len-1] != '\n');
  {
    register char *pc=line+len;
    while (isspace(*--pc))
      ;
    *(++pc) = '\0';
  }
  /* line = realloc(line, pc+1-line); */
  return line;
}

Tape *new_tape(void)
{
  int i;
  Tape *tp=(Tape *)malloc(sizeof(Tape));
  if (!tp) return NULL;
  for (i=0; i<2; i++) {
    tp->artist[i]=tp->title[i]=NULL;
    tp->songs[i]=NULL;
    tp->nsongs[i]=0;
  }
  tp->wide=0;
  return tp;
}

Tape *get_tape(FILE* fp)
{
  int i;
  Strlist *s;
  Tape *tp=new_tape();

  /**/fprintf(stderr, "get_tape() entered\n");

  if (!tp || feof(fp)) return NULL;
  for (i=0; i<2; i++) {
    tp->artist[i]=getline(fp);
    if (!tp->artist[i]) return tp;
    if (!*tp->artist[i])
      { free (tp->artist[i]); tp->artist[i]=NULL; }
    tp->title[i]=getline(fp);
    if (!tp->title[i]) return tp;
    if (!*tp->title[i])
      { free (tp->title[i]); tp->title[i]=NULL; }
    getline(fp); /* Throw away noise-reduction data */
    tp->songs[i]=(Strlist *)malloc(sizeof(Strlist));
    for (s=tp->songs[i]; s; s=s->next) {
      s->str=getline(fp);
      if (!s->str) { /* EOF */
        s->next=NULL;
        return tp;
      }
      if (!strcmp(s->str, "..")) {
        free(s->str);
        s->str=NULL;
        s->next=NULL;
        return tp;
      }
      if (!strcmp(s->str, ".")) {
        free(s->str);
        s->str=NULL;
        break;
      }
      s->next=(Strlist *)malloc(sizeof(Strlist));
        /* Error checking done at beginning of next pass */
      tp->nsongs[i]++;
    }
    s->next=NULL;
  }
  /**/fprintf(stderr, "get_tape() exit\n");
  return tp;
}

void print_preamble(void)
{
  char const * const *p;
  char timestr[100];
  time_t ct;
  struct tm *ctime;
  time(&ct);
  ctime = localtime(&ct);
  strftime(timestr, sizeof timestr,
           "%%%%CreationDate: %A %-d %B %Y, %H:%M:%S %Z", ctime);
  puts("%!PS-Adobe-1.0\n"
       "%%Creator: Toby's Fantastic Cassette Labeller (c) 1994 90tms\n"
       "%%Title: Cassette Labels");
  puts(timestr);
/*    fflush(stdout); */
/*    system("date '+%%%%CreationDate: %a %d %b %Y, %H:%M:%S %Z'"); */
/*    puts(); */
  for (p = ps_preamble;  *p;  p++) {
    puts(*p);
    fflush(stdout);
  }
  return;
}

void print_literal(register const char *cp)
{
  putchar('(');
  while (*cp)
    switch (*cp) {
    case '(':
    case ')':
      putchar ('\\');
      putchar (*cp++);
      break;
    case '\\':
      putchar (*cp++);
      putchar (*cp?*cp++:'\\'); /* quote if terminal */
      break;
    default:
      if (*cp & 0x80)
        printf("\\%03o", (unsigned int)(unsigned char)*cp++);
      else
        putchar(*cp++);
    }
  putchar(')');
  putchar(' ');
  return;
}

void print_title(const char *s)
{
  print_literal(s);
  puts(" 174 leftfituline");
  return;
}

void print_songs(Strlist *s, int n, float step)
{
  printf("%.2f [", step);
  for (;putchar('\n'), s && s->str && n>0; s=s->next, n--)
    print_literal(s->str);
  puts("] dosongs");
  return;
}

void output_page(Tape *pages[4])
{
  static int pageno=0;
  int i;
  Tape *tp;

  /**/fprintf(stderr, "output_page() entered\n");

  pageno++;
  printf("%%%%Page: %d %d\n", pageno, pageno);
  puts("20 100 translate");
  for (i=0; i<=3; i++) {
      tp=pages[i];
      if (!tp) continue;
      puts("gsave");
      switch (i) {
      case 0:
      puts("0 288 translate");
      break;
      case 1:
      /*puts("258 288 translate");*/
      puts("552 576 translate");
      puts("180 rotate");
      break;
      case 3:
      /*puts("258 0 translate");*/
      puts("552 288 translate");
      puts("180 rotate");
      break;
      default:
      break;
      }
      if (tp->nsongs[1] == 0) /* no side 2 */
      if (tp->nsongs[0]<=22) { /* no squashing */
          int x=(22-tp->nsongs[0])*4;
          printf("96 %d moveto\n", 276-x);
          print_title(tp->title[0]);
          print_songs(tp->songs[0], tp->nsongs[0], 12);
      } else if (tp->nsongs[0] <= 25) { /* squeeze */
          puts("96 276 moveto");
          print_title(tp->title[0]);
          print_songs(tp->songs[0], tp->nsongs[0], 264.0/tp->nsongs[0]);
      } else if (tp->nsongs[0] <= 44) { /* wide */
          puts("96 276 moveto");
          print_title(tp->title[0]);
          print_songs(tp->songs[0], 22, 12);
          puts("282 264 moveto");
          print_songs(followlist(tp->songs[0],22), tp->nsongs[0]-22, 12);
      } else { /* too big */
          fprintf(stderr, "%s: too many songs in %s\n",
              progname, tp->title[0]);
          puts("grestore");
          continue;
      }
      else /* 2 sides */
      if (tp->nsongs[0]+tp->nsongs[1]<=20) {
          int x=(20-tp->nsongs[0]-tp->nsongs[1])*3;
          printf("96 %d moveto\n", 276-x);
          print_title(tp->title[0]);
          print_songs(tp->songs[0], tp->nsongs[0], 12);
          printf("0 -%d rmoveto\n", x+12);
          print_title(tp->title[1]);
          print_songs(tp->songs[1], tp->nsongs[1], 12);
      } else if (tp->nsongs[0]<=22 && tp->nsongs[1]<=22) {
          int x=(22-tp->nsongs[0])*4;
          printf("96 %d moveto\n", 276-x);
          print_title(tp->title[0]);
          print_songs(tp->songs[0], tp->nsongs[0], 12);
          x=(22-tp->nsongs[1])*4;
          printf("282 %d moveto\n", 276-x);
          print_title(tp->title[1]);
          print_songs(tp->songs[1], tp->nsongs[1], 12);
      } else {
          fprintf(stderr, "%s: tape too long - %s/%s\n",
              progname, tp->title[0], tp->title[1]);
          puts("grestore");
          continue;
      }
      print_literal(tp->artist[0]);
      if (tp->artist[1]) {
      print_literal(tp->artist[1]);
      puts("false doartist");
      } else
      puts("true doartist");
      print_literal(tp->title[0]);
      if (tp->title[1]) {
      print_literal(tp->title[1]);
      puts("false dotitle");
      } else
      puts("true dotitle");
      if (tp->wide)
      puts("true doframe");
      else
      puts("false doframe");
      puts("grestore");
  }
  puts("showpage\n");
  fflush(stdout);
  return;
}

void clear_page(Tape *pages[4])
{
  int i,j;
  Strlist *s, *t;
  Tape *tp;

  for (i=0; i<4; i++) {
    tp=pages[i];
    for (j=0; j<2; j++) {
      free(tp->artist[j]);
      free(tp->title[j]);
      for (s=t=tp->songs[j]; s; t=s) {
        s=s->next;
        free(t->str);
        free(t);
      }
    }
    free(tp);
    pages[i]=NULL;
  }
}


void print_postamble(Tape *pages[8])
{
  if (pages[0])
    output_page(pages);
  if (pages[4])
    output_page(pages+4);
  fflush(stdout);
}


void add_tape(Tape *pages[8], Tape *tp)
{
  int i,sum;

  if (tp->artist[0] && tp->artist[1] && !strcmp(tp->artist[0], tp->artist[1])) {
    free(tp->artist[1]);
    tp->artist[1]=NULL;
  }
  tp->wide = (tp->nsongs[1]
              ?(tp->nsongs[0]+tp->nsongs[1]>20)
              :(tp->nsongs[0]>=26));
  if (tp->wide) {
    for (i=0; i<=7; i+=2)
      if (!pages[i])
        break; /*success*/
  } else /* !tp->wide */
    for (i=0; i<=7; i++) {
      if (!pages[i])
        break; /*success*/
      if (pages[i]->wide)
        i++;
    }
  if (i>7)
    fprintf(stderr, "%s: page space error ## CAN'T HAPPEN ##\n", progname);
  pages[i]=tp;

  /* Now see if any ready to print */
  pages += i&~3;
  sum=0;
  for (i=0; i<=3; i++)
    if (pages[i])
      sum += 1+pages[i]->wide;
  if (sum>4)
    fprintf(stderr, "%s: page double-booking error ## CAN'T HAPPEN ##\n",
            progname);
  if (sum<4)
    return; /* OK */
  /* else oputput & reset page */
  output_page(pages);
  clear_page(pages);
  return;
}

int main(int argc, char **argv)
{
  static Tape *pages[8] = {0,0,0,0,0,0,0,0};
  int i;

  progname=argv[0];
  print_preamble();
  for (i=1; i<argc; i++) {
    Tape *tp;
    FILE *fp=fopen(argv[i], "r");
    if (!fp) {
      fprintf(stderr, "%s: couldn't open %s: %s\n",
              progname, argv[i], strerror(errno));
      continue;
    }
    while ((tp=get_tape(fp))) /* ASSIGNMENT */
      add_tape(pages, tp);
    fclose(fp);
  }
  print_postamble(pages);
  return 0;
}

static char const * const ps_preamble[] =
{
  "%%PageOrder: Ascend\n"
  "%%BoundingBox: 37 99 579 675\n"
  "%%DocumentFonts: Helvetica Helvetica-Bold\n"
  "%%EndComments\n"
  "%%BeginProcSet: cassette\n",

  /* ISO 8859-1 stuff */
  "% ISOLatin1Encoding stolen from ps_init.ps in GhostScript 2.6.1.4:\n"
  "% If the ISOLatin1Encoding vector isn't known, define it.\n"
  "/ISOLatin1Encoding where { pop } {\n"
  "% Define the ISO Latin-1 encoding vector.\n"
  "% The first half is the same as the standard encoding,\n"
  "% except for minus instead of hyphen at code 055.\n"
  "/ISOLatin1Encoding\n"
  "StandardEncoding 0 45 getinterval aload pop\n"
  "    /minus\n"
  "StandardEncoding 46 82 getinterval aload pop\n"
  "%*** NOTE: the following are missing in the Adobe documentation,\n"
  "%*** but appear in the displayed table:\n"
  "%*** macron at 0225, dieresis at 0230, cedilla at 0233, space at 0240.\n"
  "% \\20x\n"
  "    /.notdef /.notdef /.notdef /.notdef /.notdef /.notdef /.notdef /.notdef\n"
  "    /.notdef /.notdef /.notdef /.notdef /.notdef /.notdef /.notdef /.notdef\n"
  "    /dotlessi /grave /acute /circumflex /tilde /macron /breve /dotaccent\n"
  "    /dieresis /.notdef /ring /cedilla /.notdef /hungarumlaut /ogonek /caron\n"
  "% \\24x\n"
  "    /space /exclamdown /cent /sterling\n"
  "       /currency /yen /brokenbar /section\n"
  "    /dieresis /copyright /ordfeminine /guillemotleft\n"
  "       /logicalnot /hyphen /registered /macron\n"
  "    /degree /plusminus /twosuperior /threesuperior\n"
  "       /acute /mu /paragraph /periodcentered\n"
  "    /cedilla /onesuperior /ordmasculine /guillemotright\n"
  "       /onequarter /onehalf /threequarters /questiondown\n"
  "% \\30x\n"
  "    /Agrave /Aacute /Acircumflex /Atilde\n"
  "       /Adieresis /Aring /AE /Ccedilla\n"
  "    /Egrave /Eacute /Ecircumflex /Edieresis\n"
  "       /Igrave /Iacute /Icircumflex /Idieresis\n"
  "    /Eth /Ntilde /Ograve /Oacute\n"
  "       /Ocircumflex /Otilde /Odieresis /multiply\n"
  "    /Oslash /Ugrave /Uacute /Ucircumflex\n"
  "       /Udieresis /Yacute /Thorn /germandbls\n"
  "% \\34x\n"
  "    /agrave /aacute /acircumflex /atilde\n"
  "       /adieresis /aring /ae /ccedilla\n"
  "    /egrave /eacute /ecircumflex /edieresis\n"
  "       /igrave /iacute /icircumflex /idieresis\n"
  "    /eth /ntilde /ograve /oacute\n"
  "       /ocircumflex /otilde /odieresis /divide\n"
  "    /oslash /ugrave /uacute /ucircumflex\n"
  "       /udieresis /yacute /thorn /ydieresis\n"
  "256 packedarray def\n"
  "} ifelse\n"
  "\n"

  "/reencodeFontISO { %def\n"
  "  dup\n"
  "  length 5 add dict                    % Make a new font (a new dict\n"
  "                                       % the same size as the old\n"
  "                                       % one) with room for our new\n"
  "                                       % symbols.\n"
  "\n"
  "  begin                                % Make the new font the\n"
  "                                       % current dictionary.\n"
  "\n"
  "\n"
  "    { 1 index /FID ne\n"
  "      { def } { pop pop } ifelse\n"
  "    } forall                           % Copy each of the symbols\n"
  "                                       % from the old dictionary to\n"
  "                                       % the new except for the font\n"
  "                                       % ID.\n"
  "\n"
  "    /Encoding ISOLatin1Encoding def    % Override the encoding with\n"
  "                                       % the ISOLatin1 encoding.\n"
  "\n"
  "    % Use the font's bounding box to determine the ascent, descent,\n"
  "    % and overall height; don't forget that these values have to be\n"
  "    % transformed using the font's matrix.\n"
  "    FontBBox\n"
  "    FontMatrix transform /Ascent exch def pop\n"
  "    FontMatrix transform /Descent exch def pop\n"
  "    /FontHeight Ascent Descent sub def\n"
  "\n"
  "    % Define these in case they're not in the FontInfo (also, here\n"
  "    % they're easier to get to.\n"
  "    /UnderlinePosition 1 def\n"
  "    /UnderlineThickness 1 def\n"
  "\n"
  "    % Get the underline position and thickness if they're defined.\n"
  "    currentdict /FontInfo known {\n"
  "      FontInfo\n"
  "\n"
  "      dup /UnderlinePosition known {\n"
  "       dup /UnderlinePosition get\n"
  "       0 exch FontMatrix transform exch pop\n"
  "       /UnderlinePosition exch def\n"
  "      } if\n"
  "\n"
  "      dup /UnderlineThickness known {\n"
  "       /UnderlineThickness get\n"
  "       0 exch FontMatrix transform exch pop\n"
  "       /UnderlineThickness exch def\n"
  "      } if\n"
  "\n"
  "    } if\n"
  "\n"
  "    currentdict                        % Leave the new font on the\n"
  "                                       % stack\n"
  "\n"
  "    end                                % Stop using the font as the\n"
  "                                       % current dictionary.\n"
  "\n"
  "    definefont                         % Put the font into the font\n"
  "                                       % dictionary\n"
  "\n"
  "    pop                                % Discard the returned font.\n"
  "} bind def\n"
  "\n"
  /* end of ISO 8859-1 stuff */
  "\n"

  "/ISO-Helvetica /Helvetica findfont reencodeFontISO\n"
  "/ISO-Helvetica findfont 12 scalefont /fn12 exch def\n"
  "/ISO-Helvetica-Bold /Helvetica-Bold findfont reencodeFontISO \n"
  "/ISO-Helvetica-Bold findfont dup\n"
  "12 scalefont /fb12 exch def\n"
  "24 scalefont /fb24 exch def\n"
  "\n"

  "/centerfit {\n" /* str max */
  " gsave\n"
  " exch dup stringwidth pop dup\n" /* max str wid wid */
  " 3 index\n"
  " exch 5 -1 roll\n" /* str wid max wid max */
  " le {\n"
  "  pop 2 div neg 0 rmoveto\n"
  " }{\n"
  "  dup 2 div neg 0 rmoveto\n"
  "  exch div 1 scale\n"
  " } ifelse\n"
  " show\n"
  " grestore\n"
  "} bind def\n"
  "\n"
  "/doartist {\n"
  " gsave\n"
  " 58 288 translate -90 rotate\n"
  " fb24 setfont\n"
  " {\n"
  "  dup 144 12 moveto 276 centerfit\n"
  "  144 -24 moveto 276 centerfit\n"
  " }{\n"
  "  dup 216 12 moveto 132 centerfit\n"
  "  216 -24 moveto 132 centerfit \n"
  "  dup 72 12 moveto 132 centerfit\n"
  "  72 -24 moveto 132 centerfit\n"
  " } ifelse\n"
  " grestore\n"
  "} bind def\n"
  "\n"
  "/dotitle {\n"
  " gsave\n"
  " 56 288 translate -90 rotate\n"
  " fn12 setfont\n"
  " {\n"
  "  dup 144 0 moveto 276 centerfit\n"
  "  144 -36 moveto 276 centerfit\n"
  " }{\n"
  "  dup 216 0 moveto 132 centerfit\n"
  "  216 -36 moveto 132 centerfit \n"
  "  dup 72 0 moveto 132 centerfit\n"
  "  72 -36 moveto 132 centerfit\n"
  " } ifelse\n"
  " grestore\n"
  "} bind def\n"
  "\n"
  "/leftfit {\n" /* str max */
  " gsave\n"
  " 1 index stringwidth pop exch\n" /* str wid max */
  " 1 index 1 index\n" /* str wid max wid max */
  " le {\n"
  "  pop pop\n"
  " }{\n"
  "  exch div 1 scale\n"
  " } ifelse\n"
  " show\n"
  " grestore\n"
  "} bind def\n"
  "\n"
  "/leftfituline {\n"
  " gsave\n"
  " fb12 setfont\n"
  " 1 index stringwidth pop\n"
  " 1 index 1 index\n"
  " le {\n"
  "  div\n"
  "  1 scale\n"
  " }{\n"
  "  pop pop\n"
  " } ifelse\n"
  " dup stringwidth pop\n"
  " gsave\n"
  " 0 rlineto stroke\n"
  " grestore\n"
  " show\n"
  " grestore\n"
  " 0 -12 rmoveto\n"
  "} bind def\n"
  "\n"
  "/dosongs {\n"
  " /linestep 3 -1 roll neg def\n"
  " fn12 setfont\n"
  " {\n"
  "  currentpoint 3 -1 roll\n"
  "  174 leftfit\n"
  "  moveto\n"
  "  0 linestep rmoveto\n"
  " } forall\n"
  "} def\n"
  "\n"
  "/doframe {\n"
  " 54 0 moveto 0 288 rlineto stroke\n"
  " 90 0 moveto 0 288 rlineto stroke\n"
  " 18 0 moveto\n"
  " 18 288 lineto 276 288 lineto 276 0 lineto\n"
  " closepath stroke\n"
  " {\n"
  "  276 0 moveto 190 0 rlineto 0 288 rlineto\n"
  "  -190 0 rlineto stroke\n"
  " } if\n"
  "} bind def\n\n"
  "\n"
  "%%EndProcSet\n"
  "\n"

  "statusdict begin\n"
  "  false setduplexmode\n"
  "end\n"
  "%%EndProlog\n"
  "%%BeginSetup\n"
  "%%PaperSize: A4\n"
  "%%EndSetup\n"
  "\n",

  NULL
};
\$\endgroup\$
  • \$\begingroup\$ Nice to see PostScript content! \$\endgroup\$ – luser droog Sep 19 '18 at 1:51
5
\$\begingroup\$

This is an interesting time capsule. I have come across my own code decades after I wrote it and thought "wow, what horrible code!" Yours doesn't seem too bad, but of course, there's room for improvement.

Don't use feof() incorrectly

As this question explains fairly well, using feof() only tells you if the previous read encountered the end of the file, and nothing has been read from the file handle when get_tape() is called. For that reason, this line

if (!tp || feof(fp)) return NULL;

could be written like this instead:

if (!tp) {
    return NULL;
}

If the feof check is used, it should be before memory is allocated for a new Tape.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

if (!*tp->artist[i])
  { free (tp->artist[i]); tp->artist[i]=NULL; }

most people find it more easily readable if you use more space:

if (!*tp->artist[i]) { 
    free(tp->artist[i]); 
    tp->artist[i]=NULL; 
}

Fix the bug (#1)

There is a subtle bug in the program. In the getline() routine (which I've renamed getoneline() to avoid conflict with getline in stdio.h) we have these few lines:

register char *pc=line+len;
while (isspace(*--pc))
  ;
*(++pc) = '\0';

The problem is that if the line was a single newline character, the second time isspace is called, it will be pointing one byte before line which is undefined behavior. I'd rewrite those lines like this:

for (char *pc=line+len-1; pc >= line && isspace(*pc); --pc) {
    *pc = '\0';
}

Fix the bug (#2)

If we have .. as the last line of the input file, which signals "end of tape" according to your description, we have a segfault and crash. The reason is that after the .. is read, the loop in main creates a second Tape with the default NULL values. Later, when print_title is called, it segfaults because the NULL is derefernced in print_literal. This could easily be fixed by simply bailing early out of print_literal if the passed pointer is NULL. In general, the program is rather fragile when the input doesn't exactly match what it's expecting.

Don't leak memory

The Tape structures are allocated but never freed which is a memory leak. I'd suggest adding calls to clear_page from within print_postamble since we know we're done with all pages at the time that's called. Also, the noise-reduction line that's read should be freed so instead of this:

getoneline(fp);   /* Throw away noise-reduction data */

it should be this:

free(getoneline(fp));   /* Throw away noise-reduction data */

Or alternatively, create another function ignoreLine that would do just that without allocating memory.

Avoid the use of global variables

In the code, progname and is declared as global variables rather than as a local variable in main. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable. I'd make progname an additional input parameter where needed and eliminate the global variable.

Rethink the division of labor

As you've noted there are some things done in PostScript and some in C and magic numbers everywhere. We can do better! Specifically, I'd recommend letting PostScript do more and have the C program do less. The reason is that Postscript is quite good at layout, translation and rotation. For that reason, I'd be inclined to simply have the C program print something like this in PostScript:

(Artist Name) [ 
    (Side A Name) [ (track 1) (track 2) (track 3) ] 
    (Side B Name) [ (track 1) (track 2) (track 3) (track 4) ] 
] makeWideCassetteLabel
(Artist Name) [ 
    (Side A Name) [ (track 1) (track 2) (track 3) ] 
] makeCassetteLabel

Then, of course, we'd need to define those PostScript functions. Which leads to a somewhat more general comment on the PostScript code.

Create smaller, simpler PostScript functions

Smaller, simpler functions can greatly ease understanding and reduce "magic number" syndrome. As an example, consider this function:

/doframe {
 54 0 moveto 0 288 rlineto stroke
 90 0 moveto 0 288 rlineto stroke
 18 0 moveto
 18 288 lineto 276 288 lineto 276 0 lineto
 closepath stroke
 {
  276 0 moveto 190 0 rlineto 0 288 rlineto
  -190 0 rlineto stroke
 } if
} bind def

Essentially what this does is draw three, or possibly 4 boxes. First, let's create a simple function:

% Draw box at current position with passed width and height
% width height -- 
/box { 1 index 0 rlineto 0 exch rlineto neg 0 rlineto closepath stroke } bind def

The first thing we have is comments. The first says what it does and the second is a stack comment which is a very common technique in PostScript and in Forth. Essentially, it says what's expected to be on the stack before the call and what's on the stack after (with -- being the dividing line).

The second is that we have a very simple function that just draws a box. Now we can rewrite doframme:

/doframe {
    18 0 moveto 36 288 box
    54 0 moveto 36 288 box
    90 0 moveto 180 288 box
    {
        270 0 moveto 180 288 box
    } if
} bind def

We can make things even simpler if we terminate each box with a move to the lower right corner. So, redefining box:

% Draw box at current position (x, y) with passed width and height, 
% updating current point to (x + width, y)
% width height -- 
/box { 1 index 0 rlineto 
    currentpoint 4 2 roll   % save this point for the end
    0 exch rlineto 
    neg 0 rlineto 
    closepath stroke 
    moveto                  % restore location
    } bind def

Now our doframe is even simpler:

/doframe {
    18 0 moveto 
    36 288 box
    36 288 box
    180 288 box
    {
        180 288 box
    } if
} bind def

It should also be apparent that if we move the moveto outside of doframe, we can easily draw the frame anywhere on the page since all of the subsequent moves and lines are relative. By thinking carefully about how these functions are used, we can better restructure them to simplify the code and to enhance understanding.

\$\endgroup\$
  • \$\begingroup\$ Note: char *pc=line+len-1; --> pathological or hostile input can result in len == 0 (first character read is a null character), then pc=line+len-1 is UB. isspace(*pc) is UB when *pc < 0. \$\endgroup\$ – chux - Reinstate Monica Sep 15 '18 at 2:19
  • \$\begingroup\$ Thanks - a great review. The "stack comment" is a technique I use now, but hadn't seen at that time. It's good to have some thoughts on the division of work between languages; IIRC, I started with almost everything in C, then moved just the bits that needed font metrics, but a more coherent (i.e. planned) design would be more like you show here. \$\endgroup\$ – Toby Speight Sep 17 '18 at 8:00
4
\$\begingroup\$

Some things I would do differently now:

  • Don't name a function getline().
  • Don't cast the return value from malloc() and realloc().
  • Prefer expressions rather than types in malloc(sizeof ...).
  • Don't bother using register qualifier - compiler can make better choices.
  • ps_preamble can be a single string - no need for an array.
  • There are far too many magic numbers in the C and the Postscript.
  • I seem to have left debugging prints in there - they are easily found, as they are marked with empty comments /**/.
  • There's no need for the fflush() calls.
  • A complete lack of unit-tests or stress-tests.
  • Use %hho instead of casting in print_literal() (perhaps not a thing in C89?).
\$\endgroup\$
3
\$\begingroup\$
  • Formatting of certain lines, like

        if (!line) return NULL;
    

    goes without saying. Same for not using curly braces around one-liners.

    I guess you already know that.

  • line = (char *)realloc(line, size); is considered bad practice. Shall realloc fail, you'd lose information already collected, plus you'd have a memory leak.

  • Error handling in getline is, umm, pessimistic:

    if (!fgets(line+len, size-len, fp))
      { free(line); return NULL; }
    

    may throw away a perfectly valid line (e.g. if the file is missing a trailing '\n').

  • The repeated 22 in

      print_songs(tp->songs[0], 22, 12);
      ....
      print_songs(followlist(tp->songs[0],22), tp->nsongs[0]-22, 12);
    

    suggests that the second call picks up where the first one ended. It looks like print_song should return a pointer to the first non-printed node, rather than be void about it.

  • Everything marked CAN'T HAPPEN eventually happens. If you print it anyway, you should not proceed.

  • The decision making should not be delegated deep into the calling sequence. For example, add_tape shall not decide wether to print or not. It seems that the code following the /* Now see if any ready to print */ comment should he factored out into a function, which is to be called from main loop.

  • No review of the postscript code, sorry.

\$\endgroup\$
  • \$\begingroup\$ In my opinion, if (!line) return NULL; is acceptable as a one-liner. \$\endgroup\$ – 200_success Sep 14 '18 at 20:20
  • \$\begingroup\$ @200_success This is a respectable opinion. In fact, at least one famous repo insists on this style (and on omitting curlies around one-liners in general). I differ. I stopped contributing to that repo. Sorry for the rant. Lets agree to disagree. \$\endgroup\$ – vnp Sep 14 '18 at 20:35
3
\$\begingroup\$

To add to @Toby Speight good self answer:

Avoid mixed signed types

Alos, with array sizing and indexing, best to use size_t.

//const int bufinc = 20;
const size_t bufinc = 20;
size_t size = 0;
//int len = 0;
size_t len = 0;
...
size += bufinc;

Questionable specifier flag

//                                                       v
strftime(timestr, sizeof timestr, "%%%%CreationDate: %A %-d %B %Y, %H:%M:%S %Z", ctime);

Prefer {}

    //if (*cp & 0x80)
    //  printf("\\%03o", (unsigned int) (unsigned char) *cp++);
    //else
    //  putchar(*cp++);

    if (*cp & 0x80) {
      printf("\\%03o", (unsigned int) (unsigned char) *cp++);
    } else {
      putchar(*cp++);
    }

Use double unless float needed for space/speed

double is more the natural FP type in C.
Below, tp->nsongs[0] converts to double, divides with 264.0 and then the double quotient is converted to float and passed to print_songs() as step. step converts to double and then passed to printf(). Hope about just staying with double and avoid the 2 conversions?

// void print_songs(Strlist *s, int n, float step) {
void print_songs(Strlist *s, int n, double step) {
  printf("%.2f [", step);
}

print_songs(tp->songs[0], tp->nsongs[0], 264.0 / tp->nsongs[0]);


print_songs(tp->songs[0], tp->nsongs[0], 264.0f / tp->nsongs[0]);
//                                            ^

Avoid looong string literals

Such strings (> 4095) go over the minimal environmental limit. I'd expect this string data to be read from another file.

"% ISOLatin1Encoding stolen from ps_init.ps in GhostScript 2.6.1.4:\n"
"% If the ISOLatin1Encoding vector isn't known, define it.\n"
 ...
 "%%PaperSize: A4\n"
 "%%EndSetup\n"
 "\n",

Consider bool when able

Yes, original code was C89, but we do not have to stay there.

int wide; /* true if continuation pages */
bool wide; /* true if continuation pages */

Long live register!!

On 2nd thought, we can let register go to the gets() home

// register char *pc = line + len;
char *pc = line + len;

Be careful with is...() functions

is...(int x) is valid for EOF and unsigned char values. A negative char can lead to UB

// isspace(*--pc)
isspace((unsigned char) *--pc)

Consider being positive

A style issue, yet I like to avoid ! and != when able as a positive test is easier to read. Perhaps you don't not think otherwise, no?

// if (!strcmp(s->str, "..")) {
if (strcmp(s->str, "..") == 0) {

Declare later and initialize

A post C89 thing, yet instead of a dangling uninitialized object, declare it when needed. Example:

// struct tm *ctime;
// ...
// ctime = localtime(&ct);

struct tm *ctime = localtime(&ct);

Timestamps: consider ISO8601

  //"%%%%CreationDate: %A %d %B %Y, %H:%M:%S %Z", ctime);
  "%%%%CreationDate: %F %T %z", ctime);

Maybe more later, GTG

\$\endgroup\$
  • \$\begingroup\$ Thanks for this; there's some things I missed, and the long literal was something I didn't even consider. I had to re-read Document Structuring Conventions Specification to confirm there's no mandated format for CreationDate; I'd now prefer ISO-8601 over RFC-822 format (oh, and the %-d is a GNU libc extension, to omit the padding; these days, I'd just use %e to replace leading zero with space). \$\endgroup\$ – Toby Speight Sep 14 '18 at 8:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.