Closed Sqeaky closed 6 years ago
You seem to have some zealous copy pasta on line 34 of the root CMakeLists.txt.
The Readme.md seems to have gained a cmake configuration. Bold strategy.
BitFieldTools looks good logic-wise, but a couple minor styling gripes come to mind. 1) The newlines above and below the datatypes.h inclusion directive(lines 47 and 49). I don't see any constructive value for them. I DO see the value in indenting, just not the vertical spacing. 2) The access specifiers in classes/structs aren't worth their own indentation level, in my opinion. They account for only 1-3 lines per class/struct and not giving them their own indentation level helps us reclaim horizontal space (which I know you value more than I do).
My styling comments for BitFieldTools can be said of Streamlogging.h as well. In addition: 1) Line 102 (which is a comment) seems to have suffered from some zealous copy pasta. 2) Minor styling gripe, but I feel when masking the visibility of the operation is improved when there are spaces on either side of the operator. In fact I do this with most operators in executing code. It's helped me with replace operations, improved visibility, and avoids issues like the super arrow operator. That said, the masking done in the LogLevel enum may benefit from some spaces.
Lines 106 and 107 of loggingtests.h are using a TEST macro instead of a TEST_EQUAL. Also, should the filename be camel cased?
Cool, I see these and I will Make changes to correct these later today.
Fixed - line 34 of the root CMakeLists.txt. Fixed - Extra Cmake in ReadMe. Good catch, I feel sloppy letting that through Moving back access specifier and indented stuff
Reverting the file logging tests.
I will also try adding some CI to this so that we can have more assurance that this works in the general case.
:exclamation: No coverage uploaded for pull request base (
master@b3fed74
). Click here to learn what that means. The diff coverage is100%
.
@@ Coverage Diff @@
## master #7 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 3
Lines ? 46
Branches ? 0
=======================================
Hits ? 46
Misses ? 0
Partials ? 0
This builds but might not do everything ever other Jagati package does.