search

nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
Other
2.08k stars 625 forks source link

Add lint check for C++ changes #12407

Open seanbudd opened 3 years ago

seanbudd commented 3 years ago

Is your feature request related to a problem? Please describe.

Unlinted C++ code exists on the master branch of NVDA, and PRs to master can contain unlinted C++ code. Currently reviewers must manually check if a PR introduces unlinted C++ code.

Describe the solution you'd like

It would be nice to have a lint check similar to our flake8 python check that checks if a PR introduces unlinted C++ code.

It would also be good to lint our existing C++ code base, but this may run into problems as discussed on #12261

Potential tool to use: cpplint

Describe alternatives you've considered

Manual checking and linting, other tools

feerrenrut commented 3 years ago

I've been involved in the integration of cppcheck before, a static analysis tool. We also have the option to run: /analyze via MSVC. see "Compiling NVDAHelper with Debugging Options" in the readme.

However, I agree it would be best to have something that runs as part of the build. Ideally it would address not only style, but also detect common errors via static analysis.

Some overviews of common tools:

josephsl commented 3 years ago

CC @LeonardDeR, I think this is something to be aware of in your C++/WinRT research. Thanks.

feerrenrut commented 2 years ago

Building nvda with scons source nvdaHelperDebugFlags=analyze will produce warnings for the code. Many of these are highlighted in code when using Visual Studio (if paths and options are set reasonably well)

There are options to log these to an xml file. This file could then be used to add pragma directives to ignore the pre-existing warnings. Then we can set the build to run analyze for all PR builds and fail them if there are any new warnings. The in code pragmas will highlight the warnings for developers to watch out for problems / fix as we go.

feerrenrut commented 2 years ago

We might also want to consider what ruleset is used: https://docs.microsoft.com/en-us/cpp/code-quality/using-the-cpp-core-guidelines-checkers?view=msvc-170

LeonarddeR commented 2 years ago

It would also be good to lint our existing C++ code base, but this may run into problems as discussed on #12261

I don't think #12261 is relevant to C++ code. With Python, auto formatting can theoretically result into parse errors. With C++, badly linted stuff simply won't build. It makes sense to find out what are the defaults of VS Code as well. I assume Visual Studio and VS Code have similar defaults.

feerrenrut commented 2 years ago

badly linted stuff simply won't build.

There may be confusion of terms here. Formatting of C++ is one thing, and as you say low risk, should be easy to introduce (however git blame needs the ignored revision fix). Most C++ linters / checkers warn about more serious "correctness" issues. These warnings may highlight a bug, but fixing them isn't always straightforward, and may introduce other bugs, or snowball into a very large and hard to verify change.

Careful and incremental is my advice. I'd be happy for a reformat to be done automatically however, with something like ClangFormat

LeonarddeR commented 4 weeks ago

I forgot about this one. @seanbudd any plans to address it in the near future?

seanbudd commented 4 weeks ago

@LeonarddeR - it's on our roadmap on a very low priority and scheduled for late next year, so I wouldn't bet on it, but we'd like to tackle it eventually