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

[Overview] Modernization code with clang-tidy #2731

Open
SunBlack opened this issue Dec 20, 2018 · 30 comments
Open

[Overview] Modernization code with clang-tidy #2731

SunBlack opened this issue Dec 20, 2018 · 30 comments
Labels
kind: todo Type of issue

Comments

@SunBlack
Copy link
Contributor

SunBlack commented Dec 20, 2018

This issue is just to have an overview about clang-tidy modernizations.

Full list of clang-tidy checks.

Checks which redirect to another test are removed from following list.

Android:

Boost:

Bugprone:

  • bugprone-argument-comment (Nothing found)
  • bugprone-assert-side-effect (Nothing found)
  • bugprone-bool-pointer-implicit-conversion
  • bugprone-branch-clone (Nothing found)
  • bugprone-copy-constructor-init
  • bugprone-dangling-handle (Nothing found)
  • bugprone-exception-escape
  • bugprone-fold-init-type (Nothing found)
  • bugprone-forward-declaration-namespace
  • bugprone-forwarding-reference-overload (Nothing found)
  • bugprone-inaccurate-erase (Nothing found)
  • bugprone-incorrect-roundings
  • bugprone-integer-division
  • bugprone-lambda-function-name (Nothing found)
  • bugprone-macro-parentheses
  • bugprone-macro-repeated-side-effects (Nothing found)
  • bugprone-misplaced-operator-in-strlen-in-alloc (Nothing found)
  • bugprone-misplaced-widening-cast
  • bugprone-move-forwarding-reference (Nothing found)
  • bugprone-multiple-statement-macro
  • bugprone-narrowing-conversions
  • bugprone-parent-virtual-call
  • bugprone-posix-return (Nothing found)
  • bugprone-sizeof-container (Nothing found)
  • bugprone-sizeof-expression
  • bugprone-string-constructor (Nothing found)
  • bugprone-string-integer-assignment (Nothing found)
  • bugprone-string-literal-with-embedded-nul (Nothing found)
  • bugprone-suspicious-enum-usage
  • bugprone-suspicious-memset-usage (Nothing found)
  • bugprone-suspicious-missing-comma (Nothing found)
  • bugprone-suspicious-semicolon (Nothing found)
  • bugprone-suspicious-string-compare
  • bugprone-swapped-arguments (Nothing found)
  • bugprone-terminating-continue (Nothing found)
  • bugprone-throw-keyword-missing (Nothing found)
  • bugprone-too-small-loop-variable (PR: Fix bug prone loop variables that are too small #2829)
  • bugprone-undefined-memory-manipulation
  • bugprone-undelegated-constructor (Nothing found)
  • bugprone-unhandled-self-assignment (Nothing found)
  • bugprone-unused-raii (Nothing found)
  • bugprone-unused-return-value (Nothing found)
  • bugprone-use-after-move (Nothing found)
  • bugprone-virtual-near-miss

Cert:

  • cert-dcl21-cpp
  • cert-dcl50-cpp
  • cert-dcl58-cpp (Nothing found)
  • cert-env33-c (Nothing found)
  • cert-err34-c
  • cert-err52-cpp
  • cert-err58-cpp
  • cert-err60-cpp
  • cert-flp30-c
  • cert-msc50-cpp
  • cert-msc51-cpp
  • cert-oop11-cp (Nothing found)

Clang-Analyzer:

  • clang-analyzer-apiModeling.StdCLibraryFunctions (Nothing found)
  • clang-analyzer-apiModeling.TrustNonnull (Nothing found)
  • clang-analyzer-apiModeling.google.GTest (Nothing found)
  • clang-analyzer-core.CallAndMessage
  • clang-analyzer-core.DivideZero
  • clang-analyzer-core.DynamicTypePropagation (Nothing found)
  • clang-analyzer-core.NonNullParamChecker
  • clang-analyzer-core.NonnilStringConstants (Nothing found)
  • clang-analyzer-core.NullDereference
  • clang-analyzer-core.StackAddressEscape (Nothing found)
  • clang-analyzer-core.UndefinedBinaryOperatorResult (Nothing found)
  • clang-analyzer-core.VLASize (Nothing found)
  • clang-analyzer-core.builtin.BuiltinFunctions (Nothing found)
  • clang-analyzer-core.builtin.NoReturnFunctions (Nothing found)
  • clang-analyzer-core.uninitialized.ArraySubscript
  • clang-analyzer-core.uninitialized.Assign
  • clang-analyzer-core.uninitialized.Branch
  • clang-analyzer-core.uninitialized.CapturedBlockVariable (Nothing found)
  • clang-analyzer-core.uninitialized.UndefReturn
  • clang-analyzer-cplusplus.InnerPointer (Nothing found)
  • clang-analyzer-cplusplus.Move (Nothing found)
  • clang-analyzer-cplusplus.NewDelete
  • clang-analyzer-cplusplus.NewDeleteLeaks
  • clang-analyzer-cplusplus.SelfAssignment (Nothing found)
  • clang-analyzer-deadcode.DeadStores (PR: Remove dead stores #3095)
  • clang-analyzer-llvm.Conventions (check not released in a stable version of clang-tidy)
  • clang-analyzer-nullability.NullPassedToNonnull (Nothing found)
  • clang-analyzer-nullability.NullReturnedFromNonnull (Nothing found)
  • clang-analyzer-nullability.NullableDereferenced (Nothing found)
  • clang-analyzer-nullability.NullablePassedToNonnull (Nothing found)
  • clang-analyzer-nullability.NullableReturnedFromNonnull (Nothing found)
  • clang-analyzer-optin.cplusplus.VirtualCall
  • clang-analyzer-optin.mpi.MPI-Checker (Nothing found)
  • clang-analyzer-optin.osx.cocoa.localizability.EmptyLocalizationContextChecker (Nothing found)
  • clang-analyzer-optin.osx.cocoa.localizability.NonLocalizedStringChecker (Nothing found)
  • clang-analyzer-optin.performance.GCDAntipattern (Nothing found)
  • clang-analyzer-optin.performance.Padding
  • clang-analyzer-optin.portability.UnixAPI (Nothing found)
  • clang-analyzer-osx.API (Nothing found)
  • clang-analyzer-osx.NumberObjectConversion (Nothing found)
  • clang-analyzer-osx.OSObjectRetainCount (Nothing found)
  • clang-analyzer-osx.ObjCProperty (Nothing found)
  • clang-analyzer-osx.SecKeychainAPI (Nothing found)
  • clang-analyzer-osx.cocoa.AtSync (Nothing found)
  • clang-analyzer-osx.cocoa.AutoreleaseWrite (Nothing found)
  • clang-analyzer-osx.cocoa.ClassRelease (Nothing found)
  • clang-analyzer-osx.cocoa.Dealloc (Nothing found)
  • clang-analyzer-osx.cocoa.IncompatibleMethodTypes (Nothing found)
  • clang-analyzer-osx.cocoa.Loops (Nothing found)
  • clang-analyzer-osx.cocoa.MissingSuperCall (Nothing found)
  • clang-analyzer-osx.cocoa.NSAutoreleasePool (Nothing found)
  • clang-analyzer-osx.cocoa.NSError (Nothing found)
  • clang-analyzer-osx.cocoa.NilArg (Nothing found)
  • clang-analyzer-osx.cocoa.NonNilReturnValue (Nothing found)
  • clang-analyzer-osx.cocoa.ObjCGenerics (Nothing found)
  • clang-analyzer-osx.cocoa.RetainCount (Nothing found)
  • clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak (Nothing found)
  • clang-analyzer-osx.cocoa.SelfInit (Nothing found)
  • clang-analyzer-osx.cocoa.SuperDealloc (Nothing found)
  • clang-analyzer-osx.cocoa.UnusedIvars (Nothing found)
  • clang-analyzer-osx.cocoa.VariadicMethodTypes (Nothing found)
  • clang-analyzer-osx.coreFoundation.CFError (Nothing found)
  • clang-analyzer-osx.coreFoundation.CFNumber (Nothing found)
  • clang-analyzer-osx.coreFoundation.CFRetainRelease (Nothing found)
  • clang-analyzer-osx.coreFoundation.containers.OutOfBounds (Nothing found)
  • clang-analyzer-osx.coreFoundation.containers.PointerSizedValues (Nothing found)
  • clang-analyzer-security.FloatLoopCounter
  • clang-analyzer-security.insecureAPI.UncheckedReturn (Nothing found)
  • clang-analyzer-security.insecureAPI.bcmp (Nothing found)
  • clang-analyzer-security.insecureAPI.bcopy (Nothing found)
  • clang-analyzer-security.insecureAPI.bzero (Nothing found)
  • clang-analyzer-security.insecureAPI.getpw (Nothing found)
  • clang-analyzer-security.insecureAPI.gets (Nothing found)
  • clang-analyzer-security.insecureAPI.mkstemp (Nothing found)
  • clang-analyzer-security.insecureAPI.mktemp (Nothing found)
  • clang-analyzer-security.insecureAPI.rand (Nothing found)
  • clang-analyzer-security.insecureAPI.strcpy
  • clang-analyzer-security.insecureAPI.vfork (Nothing found)
  • clang-analyzer-unix.API
  • clang-analyzer-unix.Malloc
  • clang-analyzer-unix.MallocSizeof (Nothing found)
  • clang-analyzer-unix.MismatchedDeallocator
  • clang-analyzer-unix.StdCLibraryFunctions (check not released in a stable version of clang-tidy)
  • clang-analyzer-unix.Vfork (Nothing found)
  • clang-analyzer-unix.cstring.BadSizeArg (Nothing found)
  • clang-analyzer-unix.cstring.NullArg (Nothing found)
  • clang-analyzer-valist.CopyToSelf (Nothing found)
  • clang-analyzer-valist.Uninitialized (Nothing found)
  • clang-analyzer-valist.Unterminated (Nothing found)

cppcoreguidelines

  • cppcoreguidelines-avoid-goto
  • cppcoreguidelines-interfaces-global-init (Nothing found)
  • cppcoreguidelines-macro-usage
  • cppcoreguidelines-narrowing-conversions
  • cppcoreguidelines-no-malloc
  • cppcoreguidelines-owning-memory
  • cppcoreguidelines-pro-bounds-array-to-pointer-decay
  • cppcoreguidelines-pro-bounds-constant-array-index
  • cppcoreguidelines-pro-bounds-pointer-arithmetic
  • cppcoreguidelines-pro-type-const-cast
  • cppcoreguidelines-pro-type-cstyle-cast
  • cppcoreguidelines-pro-type-member-init
  • cppcoreguidelines-pro-type-reinterpret-cast
  • cppcoreguidelines-pro-type-static-cast-downcast
  • cppcoreguidelines-pro-type-union-access
  • cppcoreguidelines-pro-type-vararg
  • cppcoreguidelines-slicing
  • cppcoreguidelines-special-member-functions

hicpp:

  • hicpp-avoid-goto
  • hicpp-exception-baseclass
  • hicpp-multiway-paths-covered
  • hicpp-no-assembler (Nothing found)
  • hicpp-signed-bitwise

Misc:

  • misc-definitions-in-headers
  • misc-misplaced-const
  • misc-new-delete-overloads (Nothing found)
  • misc-non-copyable-objects (Nothing found)
  • misc-non-private-member-variables-in-classes
  • misc-redundant-expression
  • misc-static-assert (Nothing found)
  • misc-throw-by-value-catch-by-reference (Nothing found)
  • misc-unconventional-assign-operator
  • misc-uniqueptr-reset-release (Nothing found)
  • misc-unused-alias-decls (Nothing found)
  • misc-unused-parameters
  • misc-unused-using-decls

Modernize:

MPI

  • mpi-buffer-deref (Nothing found)
  • mpi-type-mismatch (Nothing found)

OpenMP

Performance

Readability:

Not relevant checks:

  • abseil-*
  • fuchsia-*
  • google-*
  • llvm-*
  • objc-*
  • portability-simd-intrinsics (experimental part of std)
  • zircon-*

In case you see a modernization we could apply or can't apply, feel free to write it as comment. In case there is a modernization that needs discussion, open a new issue and write reference to it as comment, so I can add it to this list.

@jasjuang
Copy link
Contributor

I think modernize-use-auto makes a lot of sense for iterators, but for new expressions and cast expressions to make sense, MinTypeNameLength has to be at least larger or equal the default 5.

modernize-avoid-c-arrays and modernize-deprecated-headers is definitely needed.

I think modernize-make-shared, modernize-make-unique, modernize-use-emplace, modernize-use-nullptr are also good to add in.

@taketwo
Copy link
Member

taketwo commented Dec 21, 2018

modernize-use-auto: be very careful with Eigen types. Basically, do not use the auto keywords with Eigen's expressions, unless you are 100% sure about what you are doing (ref).

@SunBlack
Copy link
Contributor Author

@jasjuang modernize-make-shared & modernize-make-unique will not have any effect, because we didn't had C++11 before ;).

@SunBlack
Copy link
Contributor Author

I added two points for discussion.

@SunBlack
Copy link
Contributor Author

SunBlack commented Feb 11, 2019

I think modernize-use-auto makes a lot of sense for iterators, but for new expressions and cast expressions to make sense, MinTypeNameLength has to be at least larger or equal the default 5.

@jasjuang Discussion is for this is currently here: #2838 - in case you want to be part of it ;)

@denix56
Copy link
Contributor

denix56 commented Feb 24, 2019

Maybe we should also replace typedef with using ? Clang also suggests it as modernization. Moreover, it is, imho, much more readable

@taketwo
Copy link
Member

taketwo commented Feb 27, 2019

It'd certainly be nice to switch to using in the long run. @SunBlack already attempted to apply automatic fix, but there were problems, see #2791.

@SunBlack
Copy link
Contributor Author

I suggest to skip modernize-use-default-member-init, because I really don't like to have default values stored in header.

@taketwo
Copy link
Member

taketwo commented Apr 24, 2019

Generally, I agree with you on this. However, due to the fact that PCL is a heavily templated library, we already have default values in ".h" files most of the times. And even if we don't have them in ".h", then they are in ".hpp", which is effectively the same. Meantime, there are benefits in using default member init. For example, when there are multiple constructors (which is often the case in PCL) it helps to maintain consistency between default values.

@SergioRAgostinho
Copy link
Member

Just wanted to add here the decision we took for readability-implicit-bool-conversion. tldr activate the option AllowPointerConditions at that time. See this comment for reference

@SunBlack
Copy link
Contributor Author

SunBlack commented May 6, 2019

Just to mention: I don't believe we can integrate clang-tidy in near feature into build server. I tried it in our project, but there are some issues:

  • I have no idea how header-filter really works. It should be a Regex, but I don't know which syntax they allow, because all my regexe works just partially (need to exclude moc and ui files of Qt from output)
  • If we switch to AUTOMOC, AUTOUI and AUTORCC, these file will not anymore generated during CMake generation phase. Instead they will be generated at start of a build (there are _autogen targets then). So we need to build all _autogen targets to get run clang-tidy (if not clang-tidy will complain missing moc/ui/rcc files). Because there is no global autogen target, we need to write a script to grep all _autogen targets of makefile, because makefiles doesn't support wildcard, so we can't call make *_autogen. Easiest solution use CMake 3.14 and CMAKE_EXPORT_COMPILE_COMMANDS=ON
  • Output of clang-tidy 6.0 / 7 / 8 produces sometimes corrupted output, if you parse console output (starts printing output a hit within another hit). So in case we want to parse output, we need to use a YAML parser (this output seems to be valid)
  • Not sure how long a run will need on Azure, could be longer than MSVC build ;).

@taketwo
Copy link
Member

taketwo commented May 7, 2019

Not sure how long a run will need on Azure, could be longer than MSVC build ;).

😱 In case it turns out to be very expensive to run, we may consider making it a periodic job which runs, say, weekly.

But the rest of the items you posted make this all sound like a very daunting task.

@SunBlack
Copy link
Contributor Author

General question, due to:

Lack of time to review. Like I said, I'm not putting any priority in reviewing these PRs because they are time consuming and not in the goals for this release. That being said, they are fairly trivial in the changes they make, so feel I'm perfectly ok in merging them with just your review.

Originally posted by @SergioRAgostinho in #3112 (comment)

Should I stop applying modernization until release of 1.10.0 or still continue?

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Jun 19, 2019

If you have time to spare, I would rather have your help on the transition to std until the release is complete. There are a couple of items now that are simply manual labor, e.g.: the bind -> lambda replacement. But let Sergey express his opinion, since he was still reviewing things.

@taketwo
Copy link
Member

taketwo commented Jun 19, 2019

I am fine reviewing/merging clang-tidy conversions. But indeed help on the milestone items would be more important.

@SunBlack
Copy link
Contributor Author

Well ok, don't know if I really have (currently) time to help on boost. There is a small thing I will change, if I have time (PCL is using boost filesystem still via iterator instead of for-ranged loop).

Nevertheless I will continue with some clang-tidy changes, because I can run clang-tidy if I don't need my PC and just need to review it after it .

@SunBlack
Copy link
Contributor Author

Updated list, so progress is better to see.

@SunBlack
Copy link
Contributor Author

SunBlack commented Jul 15, 2019

I did run this night clang-tidy with all checks on our build server, to get a better feeling, which checks will produce smaller PRs and which bigger one. Well, this did run a bit out of control 😆

Job statistic:

  • Error Log-File Size of clang-tidy: 287.71MB
  • clang-tidy time: 1h 13min
  • Java parsing time: Still not finished after > 7h => to much for the Jenkins Plugin xD.

Following stats are manually grepped via regex. So no duplicating detection was done.

General warnings statistic:

  • Total: 869 173
  • Outside of PCL code: 12 251
  • Within the 3rd-Party code of PCL: 56 167
  • Within the non-3rd-Party code of PCL: 809 755

Categories:

  • abseil: 0
  • android: 2
  • boost: 0
  • bugprone: 6 461
  • cert: 2 834
  • clang-analyzer: 791
  • cppcoreguidelines: 246 098
  • fuchsia: 153 825
  • google: 151 613
  • hicpp: 82 940
  • llvm: 46 737
  • misc: 37 543
  • modernize: 35 527
  • mpi: 0
  • objc: 0
  • performance: 1 367
  • portability: 2 690
  • readability: 100 747
  • zircon: 0

@SergioRAgostinho
Copy link
Member

Sheer curiosity: what exactly is your company doing with PCL? I'm curious on the reasons for subjecting all your code base to this extensive battery of checks. 👀

@SunBlack
Copy link
Contributor Author

Sheer curiosity: what exactly is your company doing with PCL?

We are not using this much modules of PCL. Most reason to use PCL for us: If we want to experiment with data we have here already a lot of algorithms, so we don't need to implement everything on our own.

I'm curious on the reasons for subjecting all your code base to this extensive battery of checks. 👀

We are working with a lot of students, so we have a lot of developer who only spend a few month/years on our project. So automatic checks are perfect to reduce time we maintainer have to spend on MRs to get the code ready to merge. In general: My main task in our project is to refactor code (I'm doing it since some years now), so I have less work later if the code kept clean automatically (Build-
server with Google Test, warnings as errors, CppCheck, clang-tidy and in near future clang-format).

And clang-tidy is nice, because e.g. when to use std::move and when const reference is sometimes hard to know, because I don't know of every class of us if it is trivially copyable or not.

@taketwo
Copy link
Member

taketwo commented Jul 16, 2019

performance: 1 367

Curious to see those :)

@SunBlack
Copy link
Contributor Author

SunBlack commented Jul 16, 2019

performance: 1 367

Curious to see those :)

With deduplication these are not this much:

  • performance-implicit-conversion-in-loop: 1
  • performance-inefficient-string-concatenation: 4
  • performance-type-promotion-in-math-fn: 2
  • performance-unnecessary-copy-initialization: 8
  • performance-unnecessary-value-param: 359

@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the status: stale label May 19, 2020
@kunaltyagi kunaltyagi added the kind: todo Type of issue label May 21, 2020
@stale stale bot removed the status: stale label May 21, 2020
@gnawme
Copy link
Contributor

gnawme commented Jul 6, 2020

Easiest way:

  • Add set(CMAKE_EXPORT_COMPILE_COMMANDS ON) to CMakeLists.txt
  • Run cmake .
  • This generates compile_commands.json
  • Use run-clang-tidy, which will look for, and iterate through, compile_commands.json and apply the specified checks
  • Policy at my employer is to run clang-tidy (with a subset of the modernize-* checks) when your branch is close to being ready to merge (running it in CI for every push is not useful)

@kunaltyagi
Copy link
Member

Is there a way to generate compile_commands.json without requiring all dependencies to be installed?

@gnawme
Copy link
Contributor

gnawme commented Jul 7, 2020

Is there a way to generate compile_commands.json without requiring all dependencies to be installed?

set(CMAKE_EXPORT_COMPILE_COMMANDS ON) just generates the compile database, I'm not sure that it's analyzing or requiring dependencies as well.

@SunBlack
Copy link
Contributor Author

SunBlack commented Jul 7, 2020

Is there a way to generate compile_commands.json without requiring all dependencies to be installed?

Yes, as it requires a successful run of CMake. Furthermore clang-tidy requires also all dependencies, to can check the code (not like CppCheck).

@gnawme
Copy link
Contributor

gnawme commented Jul 7, 2020

Is there a way to generate compile_commands.json without requiring all dependencies to be installed?

Yes, as it requires a successful run of CMake. Furthermore clang-tidy requires also all dependencies, to can check the code (not like CppCheck).

Generating compile_commands.json only requires that cmake . complete, is that what you mean? It will often complain that certain dependencies are missing, but still generate the compile database.

@SunBlack
Copy link
Contributor Author

SunBlack commented Jul 7, 2020

Generating compile_commands.json only requires that cmake . complete, is that what you mean? It will often complain that certain dependencies are missing, but still generate the compile database.

Yes, but when a dependency is missing, the target which requires these are not part of the database, as we skip some CMake code than.

@gnawme
Copy link
Contributor

gnawme commented Feb 15, 2021

Opened PR #4560, working from the base modules of the PCL dependency graph to keep the LOC changed to a manageable quantity. Future PRs will continue up the dependency graph.

Supersedes #4249 which was too large for sensible review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: todo Type of issue
Projects
None yet
Development

No branches or pull requests

7 participants