search

PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.84k stars 4.61k forks source link

[Overview] Modernization code with clang-tidy #2731

Open SunBlack opened 5 years ago

SunBlack commented 5 years ago

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:

Cert:

Clang-Analyzer:

cppcoreguidelines

hicpp:

Misc:

Modernize:

MPI

OpenMP

Performance

Readability:

Not relevant checks:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

SunBlack commented 5 years ago

I added two points for discussion.

SunBlack commented 5 years ago

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 commented 5 years ago

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

taketwo commented 5 years ago

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 commented 5 years ago

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

taketwo commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

taketwo commented 5 years ago

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

:scream: 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 commented 5 years ago

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 https://github.com/PointCloudLibrary/pcl/pull/3112#issuecomment-500226328

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

SergioRAgostinho commented 5 years ago

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 commented 5 years ago

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

SunBlack commented 5 years ago

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 commented 5 years ago

Updated list, so progress is better to see.

SunBlack commented 5 years ago

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:

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

General warnings statistic:

Categories:

SergioRAgostinho commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

performance: 1 367

Curious to see those :)

SunBlack commented 5 years ago

performance: 1 367

Curious to see those :)

With deduplication these are not this much:

stale[bot] commented 4 years ago

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

gnawme commented 4 years ago

Easiest way:

kunaltyagi commented 4 years ago

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

gnawme commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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.