CodeSOD: Mapro

Steinin was doing some work for a company that needed some geographic information systems work done. Steinin was just the programmer, and was no expert on the algorithms they needed implemented, and so asked for some references on how to implement them.

They handed him this C code.

#define get(l, row, col) \ (parm.seg ? \(segment_get(l.seg, &v, row + l.row_offset, col + l.col_offset) < 1 ? \ (sprintf(string,"r.flow: cannot read segment file for %s",l.name),\ G_fatal_error(string)) : \ v) : \l.buf[row][col])#define put(l, row, col, w) \ (parm.seg ? \(v = w, \ segment_put(l.seg, &v, row + l.row_offset, col + l.col_offset) < 1 ? \ (sprintf(string,"r.flow: cannot write segment file for %s",l.name), \ G_fatal_error(string)) : \ 0) : \(l.buf[row][col] = w))static void interpolate_border(void){ int i, r = region.rows, c = region.cols; for (i = 0; i < c; i++) {put(el, -1, i, get(el, 0, i) * 2 - get(el, 1, i));put(el, r, i, get(el, r - 1, i) * 2 - get(el, r - 2, i)); } for (i = 0; i < r; i++) {put(el, i, -1, get(el, i, 0) * 2 - get(el, i, 1));put(el, i, c, get(el, i, c - 1) * 2 - get(el, i, c - 2)); } put(el, -1, -1, 3 * get(el, 0, 0) - get(el, 0, 1) - get(el, 1, 0)); put(el, -1, c, 3 * get(el, 0, c - 1) - get(el, 0, c - 2) - get(el, 1, c - 1)); put(el, r, -1, 3 * get(el, r - 1, 0) - get(el, r - 2, 0) - get(el, r - 1, 1)); put(el, r, c, 3 * get(el, r - 1, c - 1) - get(el, r - 2, c - 1) - get(el, r - 1, c - 2));}

Now, this function works, and it does... something. Frankly, I'm not going to try and parse through the work it's doing here.

segment_get and segment_put are functions for interacting with the map data directly. They're "helpfully" wrapped up in macros that include some error handling.

Let's take a look at these macros. They all take some input parameters, but they also reference parm and v- global variables. There's also nested ternaries, which go a long way to making the code unreadable.

If param.seg is non-zero, we use segment_put or segment_get, and if that returns a value less than one, we bail with a fatal error. Oh, and look at that, another global variable, string. Otherwise, if it were successful, we return a value- either v (for get) or 0 (for put).

But, if there is no param.seg value, we just interact directly with a 2D array called l in the macro. Which is called el in interpolate_border, which is another global variable.

param.seg, as a member of a global struct, is defaulted to 0, so the default behavior of this macro is to interact with a 2D array. That's not the behavior that Steinin was trying to understand, but it was also wildly unclear where param.seg actually got set- since it's a global variable and is touched by many pieces of code.

Steinin did get what he wanted out of the sample, and was able to write a fairly high-performing system for doing "frow-routes and flow accumulation and other similar raster map operations". But it took awhile to unpick this "reference implementation", and even when he was done with it, the greybeards were skeptical that his cleaner, clearer, and less-global-variable-inhabited version could possibly perform as well as theirs.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!