cleanup suggestions - Githubissues

tkruse commented 9 years ago

Replied on reddit, but maybe better here:

Your git repo is already very clean, so some more cleanup suggestions (note I am not a C programmer):

Maybe add a README.md file to your repo root so that the project looks pretty on github. Your markdown files could maybe be more github markdown compatible (^title and snippet language, easier to read/review them on github).
It would look nice if you included continuous integration like travis, it is free and quick to set up.
The 'metrics' script does not need to be at project root, and can be named metrics.py.
The wren executable should go somewhere into the build folder.
A modern alternative to makefiles could be nice (cmake, autotools, scons) for portability.
Could be nice to doxygen your source-code.

Building wren I got make errors (Linux cc 4.8.1):

src/main.c:84:5: error: ignoring return value of ‘getline’, declared with attribute warn_unused_result

hacked that away, then could not link to math.h functions:

error: undefined reference to fmin()

had to move '-lm' to end of Makefile line.

You might be interested in the cpplint tool as well, a 2012 version is available via pypi. Maybe run it with: $ cpplint --filter=-whitespace,-readability/casting,-legal,-readability src/.c src/.h include/*.h

tkruse commented 9 years ago

Also there is OCLint, which I discovered just now. Takes an hour to compile (compiling clang I think), and requires a command.json (https://github.com/rizsotto/Bear), but then yields this report like this:

$ build/oclint-0.9.dev.02251e4/bin/oclint -disable-rule=ShortVariableName -disable-rule=UselessParentheses ../wren/src/*.c

OCLint Report

Summary: TotalFiles=8 FilesWithViolations=6 P1=0 P2=80 P3=74 

src/main.c:56:3: switch statements don't need default when fully covered P3 
src/wren_compiler.c:2202:3: switch statements don't need default when fully covered P3 
src/wren_compiler.c:1301:38: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1303:29: unused method parameter P3 The parameter 'compiler' is unused.
src/wren_compiler.c:1303:49: unused method parameter P3 The parameter 'name' is unused.
src/wren_compiler.c:1303:61: unused method parameter P3 The parameter 'length' is unused.
src/wren_compiler.c:1535:42: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1541:38: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1565:41: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1578:41: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1785:38: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1790:40: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1810:40: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1858:39: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1922:38: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1941:36: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1951:37: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1961:36: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:1998:34: unused method parameter P3 The parameter 'allowAssignment' is unused.
src/wren_compiler.c:2025:21: unused method parameter P3 The parameter 'compiler' is unused.
src/wren_compiler.c:2025:41: unused method parameter P3 The parameter 'name' is unused.
src/wren_compiler.c:2025:53: unused method parameter P3 The parameter 'length' is unused.
src/wren_compiler.c:524:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 20 exceeds limit of 10
src/wren_compiler.c:636:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 15 exceeds limit of 10
src/wren_compiler.c:686:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 39 exceeds limit of 10
src/wren_compiler.c:2198:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 74 exceeds limit of 10
src/wren_compiler.c:524:1: high ncss method P2 Method of 38 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:636:1: high ncss method P2 Method of 48 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:686:1: high ncss method P2 Method of 118 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:1296:1: high ncss method P2 Method of 35 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:2198:1: high ncss method P2 Method of 85 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:2464:1: high ncss method P2 Method of 35 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:2574:1: high ncss method P2 Method of 47 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:2717:1: high ncss method P2 Method of 35 non-commenting source statements exceeds limit of 30
src/wren_compiler.c:866:38: empty while statement P2 
src/wren_compiler.c:332:1: long method P3 Method with 54 lines exceeds limit of 50
src/wren_compiler.c:686:1: long method P3 Method with 137 lines exceeds limit of 50
src/wren_compiler.c:942:1: long method P3 Method with 53 lines exceeds limit of 50
src/wren_compiler.c:1206:1: long method P3 Method with 66 lines exceeds limit of 50
src/wren_compiler.c:1296:1: long method P3 Method with 81 lines exceeds limit of 50
src/wren_compiler.c:1605:1: long method P3 Method with 52 lines exceeds limit of 50
src/wren_compiler.c:2198:1: long method P3 Method with 95 lines exceeds limit of 50
src/wren_compiler.c:2354:1: long method P3 Method with 91 lines exceeds limit of 50
src/wren_compiler.c:2464:1: long method P3 Method with 81 lines exceeds limit of 50
src/wren_compiler.c:2574:1: long method P3 Method with 109 lines exceeds limit of 50
src/wren_compiler.c:2717:1: long method P3 Method with 55 lines exceeds limit of 50
src/wren_compiler.c:1673:5: missing break in switch statement P2 
src/wren_compiler.c:1033:3: empty do/while statement P2 
src/wren_compiler.c:1523:3: empty do/while statement P2 
src/wren_compiler.c:1685:9: empty do/while statement P2 
src/wren_compiler.c:2291:7: empty do/while statement P2 
src/wren_compiler.c:531:35: parameter reassignment P3 
src/wren_compiler.c:532:35: parameter reassignment P3 
src/wren_compiler.c:533:34: parameter reassignment P3 
src/wren_compiler.c:534:35: parameter reassignment P3 
src/wren_compiler.c:535:33: parameter reassignment P3 
src/wren_compiler.c:536:32: parameter reassignment P3 
src/wren_compiler.c:537:32: parameter reassignment P3 
src/wren_compiler.c:538:32: parameter reassignment P3 
src/wren_compiler.c:539:33: parameter reassignment P3 
src/wren_compiler.c:540:34: parameter reassignment P3 
src/wren_compiler.c:541:36: parameter reassignment P3 
src/wren_compiler.c:542:36: parameter reassignment P3 
src/wren_compiler.c:543:35: parameter reassignment P3 
src/wren_compiler.c:544:34: parameter reassignment P3 
src/wren_compiler.c:545:34: parameter reassignment P3 
src/wren_compiler.c:546:33: parameter reassignment P3 
src/wren_compiler.c:547:35: parameter reassignment P3 
src/wren_compiler.c:1436:20: parameter reassignment P3 
src/wren_compiler.c:1591:5: parameter reassignment P3 
src/wren_compiler.c:1601:3: parameter reassignment P3 
src/wren_compiler.c:2829:5: parameter reassignment P3 
src/wren_compiler.c:524:1: high npath complexity P2 NPath Complexity Number 393216 exceeds limit of 200
src/wren_compiler.c:585:3: empty else block P2 
src/wren_core.c:1005:1: high ncss method P2 Method of 488 non-commenting source statements exceeds limit of 30
src/wren_core.c:105:7: bitwise operator in conditional P2 
src/wren_core.c:1005:1: long method P3 Method with 169 lines exceeds limit of 50
src/wren_core.c:409:32: avoid branching statement as last in loop P2 
src/wren_debug.c:55:3: switch statements don't need default when fully covered P3 
src/wren_debug.c:5:31: unused method parameter P3 The parameter 'vm' is unused.
src/wren_debug.c:29:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 80 exceeds limit of 10
src/wren_debug.c:29:1: high ncss method P2 Method of 200 non-commenting source statements exceeds limit of 30
src/wren_debug.c:29:1: long method P3 Method with 228 lines exceeds limit of 50
src/wren_debug.c:29:1: high npath complexity P2 NPath Complexity Number 352 exceeds limit of 200
src/wren_debug.c:80:57: dead code P2 
src/wren_value.c:662:1: high ncss method P2 Method of 32 non-commenting source statements exceeds limit of 30
src/wren_value.c:258:7: bitwise operator in conditional P2 
src/wren_value.c:262:7: bitwise operator in conditional P2 
src/wren_value.c:269:7: bitwise operator in conditional P2 
src/wren_value.c:273:7: bitwise operator in conditional P2 
src/wren_value.c:289:7: bitwise operator in conditional P2 
src/wren_value.c:306:7: bitwise operator in conditional P2 
src/wren_value.c:385:7: bitwise operator in conditional P2 
src/wren_value.c:585:7: bitwise operator in conditional P2 
src/wren_value.c:682:7: bitwise operator in conditional P2 
src/wren_value.c:686:12: bitwise operator in conditional P2 
src/wren_value.c:160:1: too many parameters P3 Method with 11 parameters exceeds limit of 10
src/wren_value.c:52:3: empty do/while statement P2 
src/wren_value.c:327:3: empty do/while statement P2 
src/wren_value.c:565:3: switch statements should have default P3 
src/wren_value.c:597:3: switch statements should have default P3 
src/wren_value.c:692:5: switch statements should have default P3 
src/wren_vm.c:153:9: inverted logic P3 
src/wren_vm.c:1107:7: inverted logic P3 
src/wren_vm.c:567:5: deep nested block P3 Block depth of 6 exceeds limit of 5
src/wren_vm.c:666:5: deep nested block P3 Block depth of 6 exceeds limit of 5
src/wren_vm.c:22:46: unused method parameter P3 The parameter 'oldSize' is unused.
src/wren_vm.c:356:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 81 exceeds limit of 10
src/wren_vm.c:1087:1: high cyclomatic complexity P2 Cyclomatic Complexity Number 11 exceeds limit of 10
src/wren_vm.c:356:1: high ncss method P2 Method of 62 non-commenting source statements exceeds limit of 30
src/wren_vm.c:1087:1: high ncss method P2 Method of 40 non-commenting source statements exceeds limit of 30
src/wren_vm.c:153:9: bitwise operator in conditional P2 
src/wren_vm.c:818:11: bitwise operator in conditional P2 
src/wren_vm.c:827:11: bitwise operator in conditional P2 
src/wren_vm.c:845:11: bitwise operator in conditional P2 
src/wren_vm.c:869:13: bitwise operator in conditional P2 
src/wren_vm.c:980:11: bitwise operator in conditional P2 
src/wren_vm.c:1065:7: bitwise operator in conditional P2 
src/wren_vm.c:1070:7: bitwise operator in conditional P2 
src/wren_vm.c:1056:3: unnecessary else statement P3 
src/wren_vm.c:27:1: long method P3 Method with 53 lines exceeds limit of 50
src/wren_vm.c:101:1: long method P3 Method with 70 lines exceeds limit of 50
src/wren_vm.c:108:1: long method P3 Method with 70 lines exceeds limit of 50
src/wren_vm.c:356:1: long method P3 Method with 682 lines exceeds limit of 50
src/wren_vm.c:1087:1: long method P3 Method with 57 lines exceeds limit of 50
src/wren_vm.c:310:3: empty do/while statement P2 
src/wren_vm.c:538:7: empty do/while statement P2 
src/wren_vm.c:540:7: empty do/while statement P2 
src/wren_vm.c:769:7: empty do/while statement P2 
src/wren_vm.c:771:7: empty do/while statement P2 
src/wren_vm.c:780:7: empty do/while statement P2 
src/wren_vm.c:782:7: empty do/while statement P2 
src/wren_vm.c:791:7: empty do/while statement P2 
src/wren_vm.c:793:7: empty do/while statement P2 
src/wren_vm.c:946:7: empty do/while statement P2 
src/wren_vm.c:1031:7: empty do/while statement P2 
src/wren_vm.c:1036:3: empty do/while statement P2 
src/wren_vm.c:1091:3: empty do/while statement P2 
src/wren_vm.c:1094:3: empty do/while statement P2 
src/wren_vm.c:1095:3: empty do/while statement P2 
src/wren_vm.c:1097:3: empty do/while statement P2 
src/wren_vm.c:1098:3: empty do/while statement P2 
src/wren_vm.c:1100:3: empty do/while statement P2 
src/wren_vm.c:1164:3: empty do/while statement P2 
src/wren_vm.c:1165:3: empty do/while statement P2 
src/wren_vm.c:1166:3: empty do/while statement P2 
src/wren_vm.c:1174:3: empty do/while statement P2 
src/wren_vm.c:1175:3: empty do/while statement P2 
src/wren_vm.c:1176:3: empty do/while statement P2 
src/wren_vm.c:1184:3: empty do/while statement P2 
src/wren_vm.c:1192:3: empty do/while statement P2 
src/wren_vm.c:1200:3: empty do/while statement P2 
src/wren_vm.c:278:5: parameter reassignment P3 
src/wren_vm.c:1087:1: high npath complexity P2 NPath Complexity Number 1152 exceeds limit of 200

[OCLint (http://oclint.org) v0.9dev]

oclint: error: violations exceed threshold
P1=0[0] P2=80[10] P3=74[20]

Such reports can be tweaked to your code style.

munificent commented 9 years ago

Maybe add a README.md file to your repo root so that the project looks pretty on github.

Done!

Your markdown files could maybe be more github markdown compatible (^title and snippet language, easier to read/review them on github).

Yeah, that's a good idea. Doing this will take some work though, since those Markdown files are currently processed by Python Markdown and I'm not sure how GitHub-flavored it can be. I'll have to look into that.

It would look nice if you included continuous integration like travis, it is free and quick to set up.

Good idea! I took a peek and it looks like this will be easy to set up.

The 'metrics' script does not need to be at project root, and can be named metrics.py.

Done!

The wren executable should go somewhere into the build folder.

Maybe it's just me, but I kind of like that they get built to the top level directory. Makes it easier to run them.

A modern alternative to makefiles could be nice (cmake, autotools, scons) for portability.

I've gone back and forth a lot on this. The problem is that all of those are so complex. My current plan is to just hand-maintain a Makefile (since Wren is so simple, that isn't hard) and then probably maintain a separate VS solution for Windows if someone contributes one.

Could be nice to doxygen your source-code.

I intend to have the comments follow enough structure to run some kind of doc generator on it, but I'm not a fan of doxygen syntax (or at least I wasn't the last time I looked into it).

Building wren I got make errors (Linux cc 4.8.1):

Someone sent a pull request, so those should be fixed now.

munificent commented 9 years ago

Those OCLint warnings are interesting, but most of them aren't surprising. I probably will remove the unused parameter names at some point, but I find the code a bit easier to read with them in there. (The reason they're unused is because the functions are called through a function pointer and some of those functions use the parameter while others don't.)

Cheers!

tkruse commented 9 years ago

All cool.

Putting all that is created by a build into the build folder is maybe a cmake best practice. At some point you might want to package more files and create distributables for different OSes and Linux distributions, putting all the stuff that needs to be tarballed (wren, manpage, debugger, etc.) into one subfolder.

Regarding cpplint / OCLint: A main advantage for you setting those up would be that people can make pull requests, then travis runs those checks (in addition to unit tests), and then the contributors can see in the travis report where they used bad style or have likely created bugs.

BTW, the output of cpplint is:

$ cpplint --filter=-whitespace,-readability/casting,-legal,-readability,-build/header_guard,-build/include src/*.c src/*.h include/*.h 
src/main.c:41:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]
src/wren_compiler.c:1937:  Almost always, snprintf is better than strcpy  [runtime/printf] [4]
src/wren_core.c:667:  Never use sprintf.  Use snprintf instead.  [runtime/printf] [5]
src/wren_core.c:903:  Never use sprintf.  Use snprintf instead.  [runtime/printf] [5]
src/wren_utils.c:28:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]
src/wren_value.c:120:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]
src/wren_value.c:360:  Almost always, snprintf is better than strcpy  [runtime/printf] [4]
src/wren_value.c:361:  Almost always, snprintf is better than strcpy  [runtime/printf] [4]
src/wren_value.c:444:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]
src/wren_value.c:550:  Using sizeof(type).  Use sizeof(varname) instead if possible  [runtime/sizeof] [1]
src/wren_value.c:600:  Are you taking an address of a cast?  This is dangerous: could be a temp var.  Take the address before doing the cast, rather than after  [runtime/casting] [4]
src/wren_vm.c:579:  If you can, use sizeof(message) instead of 100 as the 2nd arg to snprintf.  [runtime/printf] [3]
src/wren_vm.c:632:  If you can, use sizeof(message) instead of 100 as the 2nd arg to snprintf.  [runtime/printf] [3]
src/wren_vm.c:683:  If you can, use sizeof(message) instead of 100 as the 2nd arg to snprintf.  [runtime/printf] [3]
src/wren_vm.c:735:  If you can, use sizeof(message) instead of 100 as the 2nd arg to snprintf.  [runtime/printf] [3]

I have no idea whether those would be useful for you.

munificent commented 9 years ago

A main advantage for you setting those up would be that people can make pull requests, then travis runs those checks

Oh, now that would be nice. I'm not gonna sweat it right now (since I only have a relatively small amount of free time to hack on Wren), but I definitely like the idea of more automated linting. Strangely enough, I've been spending the past few months writing an automated code formatter (for Dart), so this is close to home.

wren-lang / wren

cleanup suggestions #42