Feature #2

SVS97 · 2018-07-15T14:40:13Z

Changes to the training project.

In makefile I disabled warnings (-Wall -Wextra -Wpedantic -Werror), because it doesn't work normally with Ubuntu.
In db.c used functions, which absent in ANSI C. Proposed functions are more portable.
Added icnlude guard

thodnev · 2018-07-16T00:29:32Z

In makefile I disabled warnings (-Wall -Wextra -Wpedantic -Werror), because it doesn't work normally with Ubuntu.

People managing packages at Ubuntu are "special". They decided that putting an old as hell 6th version gcc in a new distro would be a great idea. To fix the problem, you need to install a fresh gcc:
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
sudo apt-get update
sudo apt-get install gcc-8 g++-8 -y
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-8 60 --slave /usr/bin/g++ g++ /usr/bin/g++-8
If everything worked good, you should see version 8 in gcc -v output

I agree, the -Werror option seems overkill, but it's the only option that makes build fail (for some strange reason that "special" people built even new gcc with stupid default flags). Thus no need to suppress all warnings, removing -Werror will do the business.
For other stuff, see the code review (should appear ASAP)

thodnev

Thanks for contributing!
It'll be able to merge your changes after you fix problems mentioned in review
Also please pay attention to your git history. Squash your commits before PRing to keep project history clean

thodnev · 2018-07-16T00:35:14Z

Makefile

@@ -3,7 +3,7 @@ DEPS=simpledb
 LIBS=zlib

 CC=gcc
-CFLAGS=-O2 -Wall -Wextra -Wpedantic -Werror


I agree that getting rid of -Werror seems good idea. But other options are useful indeed.
CFLAGS=-O2 -Wall -Wextra -Wpedantic

thodnev · 2018-07-16T00:40:09Z

db.c

@@ -229,10 +230,10 @@ int main(int argc, char *argv[]) {
 	}

 	if (args.query) {
-		char *qval = strdup(args.query);
+		char *qval = malloc(sizeof(args.query)); /* Function in ANSI C, more portable */


args.query is a string. So what strdup essentially does is allocates the required memory and copies passed string there, returning the starting address. See man 3 strdup for more.
A sizeof is not a function, but operator. And it would never ever calculate the length of a string. Try adding a printf("sizeof(args.query)=%d chars", (int) sizeof args.query); and see the output.
We don't care about backwards compatibility, so no need for old ANSI C and K&R C stuff, use the latest things available

thodnev · 2018-07-16T00:43:10Z

db.c

@@ -247,7 +248,7 @@ int main(int argc, char *argv[]) {
 		int lerr = E_OK;
 		switch(qtype) {
 		case Q_HELP:
-			printf(qhelp_docstring);
+			printf(qhelp_docstring,"\n");


Good find. But here qhelp_docstring works as a format specifier (the same as printf("Hello world");). So to do it proper, it could be rewritten as:

printf("%s\n", qhelp_docstring);

thodnev · 2018-07-16T00:44:06Z

simpledb.c

@@ -1,4 +1,6 @@
-#include "simpledb.h"
+#ifndef __SIMPLEDB_H__


Include guards are usually not required given that you work with modern compiler.
For more, see #1 (comment)

thodnev · 2018-07-16T00:45:06Z

simpledb.c

@@ -10,6 +12,8 @@
 #include <string.h>
 #include <errno.h>

+


Is there a good reason for 2 extra newlines. Before committing, it's better to see git diff

thodnev · 2018-07-16T00:46:15Z

simpledb.c

@@ -18,7 +22,7 @@ enum err item_read_f(FILE *fp, struct dbitem *item)
 	uint8_t buffer[sizeof(*item) + HASH_SIZE];
 	if (sizeof(buffer) != fread(buffer, 1, sizeof(buffer), fp))
 		return E_FREAD;
-	uLong crc_file = *(uLong *)buffer;
+    uLong crc_file = *(uLong *)buffer;                  /* endianness-independency for zlib */


Good find! But <tab> is 8 spaces, not 4 (as required by CodingStyle)

thodnev · 2018-07-16T00:46:58Z

simpledb.h

@@ -1,13 +1,14 @@
 #include <limits.h>
 #include <stdio.h>
-
+#define SIMPLEDB_H


useless stuff here

thodnev · 2018-07-16T00:49:12Z

simpledb.h

@@ -1,13 +1,14 @@
 #include <limits.h>
 #include <stdio.h>
-
+#define SIMPLEDB_H
 #ifdef DEBUG
 	#define dbg(X, ...)	do {					\
 		fprintf(stderr, ("LOG> " X "\n"), ## __VA_ARGS__);	\


It will be good to allow this macro to return value, i.e.

#define dbg(X, ...) ({int __ret = fprintf(stderr, ("LOG> " X "\n"), ## __VA_ARGS__); __ret;})

But it relies on GCC extensions. See this link for more

SVS97 · 2018-07-16T08:53:38Z

Thanks for your comments! I make the appropriate changes to my branch of the project. Please consider them.

SVS97 added 7 commits July 15, 2018 16:25

ANSI C function

5dca200

ANSI C function

82952a2

feature

c792edb

Iclude guard

60ab188

dbg

601e690

Endianess

5d4b110

ANSI C function

0599e21

thodnev requested changes Jul 16, 2018

View reviewed changes

Last changes

ac3dd93

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature #2

Feature #2

SVS97 commented Jul 15, 2018

thodnev commented Jul 16, 2018

thodnev left a comment

thodnev Jul 16, 2018

thodnev Jul 16, 2018

thodnev Jul 16, 2018

thodnev Jul 16, 2018

thodnev Jul 16, 2018

thodnev Jul 16, 2018

thodnev Jul 16, 2018

thodnev Jul 16, 2018

SVS97 commented Jul 16, 2018

Feature #2

Are you sure you want to change the base?

Feature #2

Conversation

SVS97 commented Jul 15, 2018

thodnev commented Jul 16, 2018

thodnev left a 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

SVS97 commented Jul 16, 2018