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

Integration scripts for Clang's Modernize and Tidy tool. #1677

Merged
merged 2 commits into from
Sep 22, 2015

Conversation

daniel-j-h
Copy link
Member

New directory: scripts/, in which small scripts for developers reside.

  • modernize: runs all cpp files through clang-modernize, respecting
    out targeted compiler versions, applying C++11 transformations, doing
    syntax checks and formatting --- in parallel.
  • tidy: runs all cpp files through clang-tidy, with selected
    warnings only, since we do not want to warn on every small detail.

Please check the talk slides for clang-tidy linked in the references!

Note: there might be a static analyzer script coming! :)

References:

@daniel-j-h
Copy link
Member Author

Static Analyzer Report:

report

# Runs the Clang Modernizer in parallel on the code base.
# Requires a compilation database in the build directory.

git ls-files '*.cpp' | xargs -I{} -P $(nproc) clang-modernize -p build -final-syntax-check -format -style=file -summary -for-compilers=clang-3.4,gcc-4.8 -include . -exclude third_party {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also work for headers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we have to pipe the .cpp file into it is that the compilation database generated by CMake only contains the compiler commands for a single TU. That is, you have commands in there like g++ main.cc -o main, which only match the .cpp files, since the headers are included by the preprocessor.

That said, the tool does work on headers we just don't have to specify them, check the initial run here:
https://github.com/Project-OSRM/osrm-backend/pull/1603/files

@TheMarex
Copy link
Member

This is good to merge. 👍 Can you ticket the non-false positives?

@daniel-j-h
Copy link
Member Author

Yeah, not sure how to do a proper integration, though. Would love to have a Travis build running for the modernizer at least, exiting with an error if it finds something. The tidy and analyzer tools are too noisy for continuous integration, probably good to do this once every other month.

New directory: `scripts/`, in which small scripts for developers reside.

- `modernize`: runs all cpp files through `clang-modernize`, respecting
  out targeted compiler versions, applying C++11 transformations, doing
  syntax checks and formatting --- in parallel.

- `tidy`: runs all cpp files through `clang-tidy`, with selected
  warnings only, since we do not want to warn on every small detail.

Please check the talk slides for `clang-tidy` linked in the references!

References:

- http://clang.llvm.org/extra/clang-tidy/
- http://llvm.org/devmtg/2014-04/PDFs/Talks/clang-tidy%20LLVM%20Euro%202014.pdf
- http://clang.llvm.org/extra/clang-tidy/checks/list.html
- #1603
This provides a wrapper script to invoke the Static Analyzer on the code
base. The script simply wraps your commands, that is you have to do the
following:

    ..scripts/analyze cmake ..
    ..scripts/analyze cmake --build .

Note: the Static Analyzer is integrated in Xcode, so if you are on a
Mac, consider using Xcode natively instead of this wrapper script that
will only give you HTML output.

Reference:

- http://clang-analyzer.llvm.org/
@daniel-j-h daniel-j-h merged commit 9deadc1 into develop Sep 22, 2015
@TheMarex TheMarex deleted the clang_tidy branch September 24, 2015 09:38
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

Successfully merging this pull request may close these issues.

2 participants