Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linting the code #218

Open
sarahec opened this issue Dec 14, 2022 · 5 comments
Open

Linting the code #218

sarahec opened this issue Dec 14, 2022 · 5 comments

Comments

@sarahec
Copy link
Contributor

sarahec commented Dec 14, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

sarahec commented Dec 15, 2022

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
Copy link
Contributor

OK, please go ahead then.

@sarahec
Copy link
Contributor Author

sarahec commented Dec 16, 2022

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

@sarahec
Copy link
Contributor Author

sarahec commented Dec 17, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants