openasip icon indicating copy to clipboard operation
openasip copied to clipboard

Linting the code

Open sarahec opened this issue 3 years ago • 5 comments

I've been running a linter (clang-tidy) over the code for the past month and testing the the results of the recommended fixes. Are you interested in any or all of these? (I have all of these working in a private branch.)

  1. In C++, replace system headers with C++ headers where appropriate. Example <math.h> to <cmath>. The compiler uses its own bundled headers, so supplying a system header can lead to subtle bugs if there are layout differences between the two.

Updates to use C++11 syntax and features:

  1. Use nullptr to replace NULL and 0 as appropriate. This makes the use of null pointers more explicit.
  2. Use auto or auto* for when initializing objects where the type is obvious to avoid redundant declarations (Foo* foo = new Foo() to auto* foo = new Foo()
  3. Use override to label method overrides (instead of using virtual to mean both "can override" and "is overridden".) virtual override means you're overriding and someone else can override that. This also silences numerous compiler warnings in Clang.
  4. Replace typedef with using as appropriate. This is syntactic sugar that makes code easier to read. typedef std::map<BitVector, unsigned int> Dictionary; becomes using Dictionary = std::map<BitVector, unsigned int>;
  5. Use =default to have the compiler generate the appropriate code for ctors, dtors, copy/move, etc. when you have to override one of those but leave the rest alone. Example:

Old code:

Foo::Foo() {} // Are we overriding this to do nothing?
Foo::~Foo() {
  // log some message
}
Foo::Foo() =default; // "We had to declare this, and want the compiler to do its normal thing"
Foo::~Foo() {
  // log some message
}

sarahec avatar Dec 14 '22 22:12 sarahec

Generally the cleanups are great and useful, but if they touch a lot of code, it can make history tracking (git blame) harder. How large is the diff?

pjaaskel avatar Dec 15 '22 13:12 pjaaskel

These are isolated single-line changes, so blame would change for only that line:

change lines/file # files
c++ headers 1.0 10
nullptr 8.7 525
auto (before) 6.4 139
auto (after) 7.6 476
override 5.6 59
using 1.1 8
=default 1.3 601

For reference, there are 1309 *.cc files and 124 *.icc files in the project.

sarahec avatar Dec 15 '22 18:12 sarahec

OK, please go ahead then.

pjaaskel avatar Dec 16 '22 14:12 pjaaskel

Will do. Please apply #219 first (UTF-8 fixes) as I cannot run the formatter on these new changes until that's fixed.

sarahec avatar Dec 16 '22 17:12 sarahec

Since the formatting script is having a bit of trouble (it sees a non-UTF8 character in the "before" line and crashes), I'm having to break up the changes and isolate the problem.

The first change -- replacing system headers with c++ headers -- is in #221 . The formatting script may have done more than you want in openasip/opset/base/base.cc and openasip/opset/base/double.cc, so let me know if I need to make adjustments (either changing the line length value from 78 or marking these as noformat).

sarahec avatar Dec 17 '22 22:12 sarahec