Add 13, 17 #2

codybartfast · 2020-05-10T20:43:56Z

No description provided.

ch5/ex-5-13-tail.c

klampworks · 2020-05-10T22:52:47Z

ch5/ex-5-13-tail.c

+#define INITIAL_BUFFSIZE (1 << 10)
+#define DEFAULT_TAILSIZE 10
+
+enum { OK, ERROR };


Since these values are being compared directly to 0 it might be a good idea to make it explicit like this enum { OK=0, ERROR };.
There is also stdbool.h which defines true and false if that appeals.

I don't think they are compared to 0 in this case, but some random idiot™ might come along and assume they can be - will change.

stdbool's not in the toolbox yet.

klampworks · 2020-05-10T22:54:07Z

ch5/ex-5-13-tail.c

+		while ((c = *ptr++) != '\0') {
+			if (!isdigit(c)) {
+				printf("error: Expected decimal integer: %s\n",
+				       argv[1] + 1);
+				return ERROR;
+			}
+			if (n > (max / 10)) {
+				printf("error: Argument too large, max: %lu\n",
+				       max);
+				return ERROR;
+			}
+			n *= 10;
+			d = (c - '0');
+			if (d > max - n) {
+				printf("error: Argument too large, max: %lu\n",
+				       max);
+				return ERROR;
+			}
+			n += d;
+		}


There is a standard library function atoi which does this too but I think this is ok.

I had a couple of concerns about atoi:

I think it's only an int and I was going trying to go overboard on supporting "unreasonable" input with ulong.

itoa might produce unexpected results if the string value were greater than the max for the type.

klampworks · 2020-05-10T22:56:14Z

ch5/ex-5-13-tail.c

+	/* copy info about current buffer before it's replaced */
+	char *pbuff = buff, *pcursor = cursor, *pbufflimit = bufflimit;
+	char *pfirstchr = *firstln;
+	ptrdiff_t pfirst2limit = pbufflimit - pfirstchr;


Maybe consider have one pointer and then making the other variables int offsets instead.
I think it will make this function much simpler.

Good point. Orignally I thought I was saving a bunch of pointer arithmatic in the char loop. But now you make me think about it... if everything is an offset then most of the time offsets can be compared directly with each other. The tiny bit of extra pointer arithmatic in the read char loop would, presumably, have virtually no overhead on a modern processor, but I do have one customer from Belgium who is already expressing concerns about performance :-)

klampworks · 2020-05-10T22:57:27Z

ch5/ex-5-13-tail.c

+		if (prevc == '\n') {
+			if (++currln == lineslimit)
+				currln = lines;
+			*currln = cursor;


It looks like the contents of *currln is not used?
Does removing this line change anything?

We need this. This is the only line that modifies the value of the line pointers (offsets?).

klampworks · 2020-05-10T23:01:10Z

ch5/ex-5-17-qsort-fields.c

+void parseargs(int argc, char *argv[])
+{
+	int i;
+	int maxsortcount = argc - 1;


What would happen if argc is 0?

Oh it'll break :-)

Tail is the only exercise so far that talked about handling 'unreasonable' input. Without that requirement I'm back in cowboy mode. I did start writing a check for 0 but then thought either check input properly or not at all, and doing it properly for this execice would be a lot of work, there are lot of ways to break it.

klampworks · 2020-05-10T23:01:31Z

ch5/ex-5-17-qsort-fields.c

+	sortidxs = malloc(maxsortcount * sizeof(int));
+	compares = malloc(maxsortcount * sizeof(CHARCOMP));
+	folds = malloc(maxsortcount * sizeof(int));
+	dirsorts = malloc(maxsortcount * sizeof(int));


What would happen if these mallocs fail?

klampworks · 2020-05-10T23:02:01Z

ch5/ex-5-17-qsort-fields.c

+			else if (reverse)
+				compare = strcmpr;
+			else
+				compare = (CHARCOMP)strcmp;


I don't think you need this cast? Does removing it cause an error?

Removing it causes a compiler warning. (char *, char *) vs (const char *, const char *). The natural solution is probably to make all the compare signatures (const char *, ...), but I don't think that int the knr toolbox.

klampworks · 2020-05-10T23:04:16Z

ch5/ex-5-17-qsort-fields.c

+	field = fieldspace;
+	fieldidx = 0;
+	for (chr = fieldspace; *chr; chr++) {
+		if (*chr == '\t' || *chr == '\n') {


What will happen if the last field does not end with \t or \n?
It looks like it will get missed out.
Is that a concern or just user error?

Just user error. "Check all input or check none".

klampworks · 2020-05-10T23:05:25Z

ch5/ex-5-17-qsort-fields.c

+	/* split line into fields and create temporary pointer for each field */
+	field = fieldspace;
+	fieldidx = 0;
+	for (chr = fieldspace; *chr; chr++) {


This assumes the input is a \0 terminated string but where is it coming from?
It looks like the readlines function from tail does not \0 terminate the string, but on the other hand I can see how \0s would interfere with that program.

Sorry my bad, I didn't include lines.c. Readlines is similar to tail, but it does add a \0. Yes tail doesn't need '\0', which, iirc, was refactor #57 once I realised this.

klampworks · 2020-05-10T23:09:48Z

ch5/ex-5-17-qsort-fields.c

+	return sortdata;
+}
+
+char *line(char *sortdata[])


Is this used? Consider adding static to functions you only intend to use in this C file so the compiler can tell you it's unused.

It is used, (line 58) to get the original line back from the sortdata after the sort.

Ah! I think I did try making some functions static but got the error about them being undefined - didn't read the error properly so removed the static again :-(

As a general principle I'm guessing if you can make file-level functions and values static you should? (Not in functions and blocks).

klampworks · 2020-05-10T23:10:00Z

ch5/ex-5-17-qsort-fields.c

+	return sortdata[nsortflds];
+}
+
+char *fieldspace(char *sortdata[])


This function doesn't appear to be used either?

Used by free in freestuff at end.

klampworks · 2020-05-10T23:12:20Z

ch5/ex-5-17-qsort-fields.c

+	char **tmpfields = malloc((maxsortidx + 2) * sizeof(char *));
+	char **sortdata = malloc((nsortflds + 2) * sizeof(char *));


Do these need to be **? You can use & to get the address of a variable at any time, you don't necessarily need use the maximum number of stars in the declaration.

Going to have to think about this one...

klampworks · 2020-05-10T23:15:01Z

ch5/ex-5-17-qsort-fields.c

+int main(int argc, char *argv[])
+{
+	int i, nlines;
+	char *buff, **lines, ***linedata;


I don't think these need to be ** and *** either.

klampworks · 2020-05-10T23:15:42Z

ch5/ex-5-17-qsort-fields.c

+	return numcmp(s2, s1);
+}
+
+void freestuff(char ***linedata, int nlines)


freestuff? I didn't know you were communist.

klampworks · 2020-05-10T23:20:16Z

ch5/ex-5-17-qsort-fields.c

+	for (i = 0; i < nlines; i++)
+		linedata[i] = getsortdata(lines[i]);
+
+	quicksort((void **)linedata, 0, nlines - 1, (VOIDCOMP)compare);


I don't think this cast is needed either. Does removing (VOIDCOMP) cause an error?

Just avoids a compiler warning.

klampworks · 2020-05-10T23:23:08Z

ch5/ex-5-17-qsort-fields.c

+int numcmp(char *s1, char *s2)
+{
+	double v1 = atof(s1);
+	double v2 = atof(s2);
+
+	return v1 < v2 ? -1 : v1 > v2 ? 1 : 0;
+}


This is fine.

But if you wanted to convert the floats outside the function, you could change it to

int numcmp(void *s1, void *s2) { double v1 = *(double*)s1; double v2 = *(double*)s2; ...

And the strcmpr version would be the same but with char instead of double.
That way the two functions would have the same signature and be assigned to the same function pointer.
Bypassing the type system when it gets in the way like this is called type erasure in C and is considered very cool.

Definately going to have to think about/play with that one.

pbruyninckx · 2020-05-11T08:14:55Z

ch5/ex-5-13-tail.c

+int main(int argc, char **argv)
+{
+	if (settailsize(argc, argv) != OK)
+		return 0;


Have you considered returning a non-0 error code in case things go wrong?

At this point in the book we're not explicitly returning values from main, I'm just doing it shut the compiler up. But since I am making it explicit it wouldn't do any harm to make it meaningful.

pbruyninckx · 2020-05-11T08:21:32Z

ch5/ex-5-13-tail.c

+	char c, *ptr;
+	unsigned int d;
+	unsigned long n = 0;


More descriptive variable names might might your code easier to read.

On a related note: For low-level code I think specifically writing out the loop invariants might help both to reduce the number of bugs, and to understand the code when you return to it. (Interested to hear what @klampworks thinks about this.)

I agree. My defense is that the book itself has very terse names. In particlar 'c' is always the name used for a single char. Also 8 space tabs! There's no room left on the screen for 2 letter var names ;-)

pbruyninckx · 2020-05-11T08:23:07Z

ch5/ex-5-13-tail.c

+	return OK;
+}
+
+int settailsize(int argc, char **argv)


I think parsearguments would be a better name to explain what this function does.

It was parseargs, then I changed it becasuse I was didn't like a funciton called 'parse...' having side effects. But given the whole program is constructed on the solid foundation of mutating shared state I should just go with the flow.

Add 13, 17

46c0bbf

codybartfast commented May 10, 2020

View reviewed changes

ch5/ex-5-13-tail.c Outdated Show resolved Hide resolved

klampworks reviewed May 10, 2020

View reviewed changes

This was referenced May 11, 2020

Make value of OK explicit #3

Closed

Offsets over pointers? #4

Open

pbruyninckx reviewed May 11, 2020

View reviewed changes

This was referenced May 11, 2020

Check result of call to malloc! #5

Closed

Use static for 'private' declarations #6

Closed

Go with the flow #7

Closed

Think #8

Closed

Think more #9

Open

homework

2f5bdd6

codybartfast merged commit efd2395 into fake-master Jul 25, 2020

		char *tmpfields = malloc((maxsortidx + 2) sizeof(char *));
		char *sortdata = malloc((nsortflds + 2) sizeof(char *));

Add 13, 17 #2

Add 13, 17 #2

Conversation

codybartfast commented May 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klampworks May 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klampworks May 10, 2020 •

edited

Loading