Line and column tracker for text files

Question

A class that keeps track of lines and columns in a text file. I use it in a parser so that when an error occurs, I can print out the line and column at which it occured.

Here's line col.hpp

#ifndef line_col_hpp
#define line_col_hpp

#include <cctype>
#include <cstdio>
#include <cassert>

///Keeps track of lines and columns in text.
///Great for writing error messages in parsers
class LineCol {
public:
  using LineType = unsigned int;
  using ColType = unsigned int;
  static const ColType SIZE_OF_TAB = 8;
  static const LineType FIRST_LINE = 1;
  static const ColType FIRST_COL = 1;

  explicit LineCol(LineType line = FIRST_LINE,
                   ColType  col  = FIRST_COL);

  ///Move line and col according to the char.
  ///Call this at the end of the loop with the char you just processed
  void update(char);
  ///This calls update(char) for each char in the null terminated string
  void update(const char *);
  ///This calls update(char) for the first n chars in the string
  void update(const char *, size_t);
  ///Sets line to FIRST_LINE and col to FIRST_COL
  void reset();

  LineType getLine() const;
  ColType getCol() const;
  const char *getStr() const;
private:
  LineType line;
  ColType col;
};

#endif

Here's line col.cpp

#include "line col.hpp"

LineCol::LineCol(LineType line, ColType col)
  : line(line), col(col) {
  assert(line >= FIRST_LINE);
  assert(col >= FIRST_COL);
}

void LineCol::update(char c) {
  switch (c) {
    case '\t':
      col += SIZE_OF_TAB;
      break;
    case '\n':
      line++;
      col = FIRST_COL;
      break;
    //vertical tab and form feed do the same thing on my system
    case '\v':
    case '\f':
      line++;
      break;
    case '\r':
      col = FIRST_COL;
      break;
    case '\b':
      //backspace doesn't move up lines
      //when backspacing tabs it treats them as spaces so it only
      //moves back one char
      if (col != FIRST_COL) {
        col--;
      }
      //i don't have to put a break here but should I even though we'll
      //fallthrough to another break?
    case '\0':
    case EOF:
      break;

    default:
      if (std::isprint(c)) {
        col++;
      }
  }
}

void LineCol::update(const char *str) {
  assert(str);
  while (*str) {
    update(*str);
    str++;
  }
}

void LineCol::update(const char *str, size_t size) {
  assert(str);
  while (size) {
    update(*str);
    str++;
    size--;
  }
}

void LineCol::reset() {
  line = FIRST_LINE;
  col = FIRST_COL;
}

LineCol::LineType LineCol::getLine() const {
  return line;
}

LineCol::ColType LineCol::getCol() const {
  return col;
}

const char *LineCol::getStr() const {
  //The largest 64 bit integer value is 18446744073709551615 which is
  //20 characters. 20 + ':' + 20 + '\0' = 42
  static char str[42];
  std::snprintf(str, 42, "%u:%u", line, col);
  return str;
}

Heres an example of how the class is used.

const char *buf;
size_t size;
LineCol lc;
for (const char *c = buf; c < buf + size; c++) {
  //do stuff with the character
  //maybe print lc.getStr()
  lc.update(*c);
}

How could this be improved? Are there any special characters that I've missed?

Answer 1

Overall I'd say this code is very easy to read. I can see quickly how everything works. Here are a few suggestions:

Only Make Public What's Necessary

There are 3 update() functions. Looking at them, it's not clear what the purpose of them is. The first one doesn't look like it should be public at all. It changes the internal state of the object, but doesn't update the string it comes from, which is confusing. It means that a caller could repeatedly call it with the same character from a string and it would count incorrectly. I'd make that version private.

I'd also make the argument to void update(char) be const since it's not modified.

Naming

Name Your Method Arguments!

What do the method arguments represent? Presumably for the 2 update() methods that take a pointer to a character, they represent a string to be parsed. They should have a name like parseString or something similar.

Improve Your Method Names

The method name update() doesn't tell me anything about what the function does. Any method that modifies the state of an object updates it in some way. A better name might be parse() or advanceLineAndColumn() or something that says what it actually does.

The getStr() method is very deceptive because it looks like it gets a pointer to the string this object is parsing. But it doesn't! Since this is C++, you'd be better served by making an operator<<() method or free function to print out your object.

Even reset() could be a little more descriptive. Perhaps something like resetParsing() or resetLineAndColumn(). Consequently, why would I want to use reset() instead of just creating a new instance? They seem fairly lightweight, so what's the advantage? If there is one, maybe that would be helpful in renaming the method.

Fix This Bug

In updated(str, size); you have a potential memory stomper. If size is greater than the actual length of str, you'll call update(str); and str might point to memory you don't have permission to read, causing a crash. Alternatively it might point to other data in your app and cause other problems such as incorrect line and column values.

All of this could be avoided if you...

Use std::string

Since you're using C++, you should be using C++'s string class to handle strings. It eliminates the possibility of running off the end of the string because you can access it using the at() method, which will throw an exception if you try to access a character outside the length of the string.