cpc / openasip

Open Application-Specific Instruction Set processor tools (OpenASIP)
http://openasip.org
Other
138 stars 41 forks source link

Linting the code #218

Open sarahec opened 1 year ago

sarahec commented 1 year ago

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
}
pjaaskel commented 1 year ago

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?

sarahec commented 1 year ago

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.

pjaaskel commented 1 year ago

OK, please go ahead then.

sarahec commented 1 year ago

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

sarahec commented 1 year ago

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).