llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.23k stars 11.65k forks source link

Tune inspections to a specific C++ standard #44390

Open llvmbot opened 4 years ago

llvmbot commented 4 years ago
Bugzilla Link 45045
Version unspecified
OS All
Reporter LLVM Bugzilla Contributor
CC @davidbolvansky

Extended Description

Please allow specifying the C++ standard for which to inspect.

It is not helpful, and is very distracting, being advised to use 'nullptr', 'auto', 'explicit', 'override', 'move', etc. if the C++ standard for your project is C98!

llvmbot commented 4 years ago

This should go some way to solving your issue https://reviews.llvm.org/D75538

llvmbot commented 4 years ago

The only conclusion I get from this discussion is that there is a break in communication between the JetBrains team developing CLion, and the team developing clang-tidy. As a user of CLion I do not run the clang-tidy command myself, and I have no idea what the settings or flags are, nor how to change them. My cmake projects explicitly set the C++ standard to use, and everything else Should Just Work.

It's worth understanding the (huge) difference between the CLion team and the community behind the LLVM project (which clang-tidy is a part of). While the former is a commercial product with certain support policies, the latter is an open-source project with a number of contributors interested in particular features and use cases.

If CLion exposes clang-tidy diagnostics in a certain way and treats it as a 3rd party tool (i.e. their support doesn't take the burden of investigating the issues and having them reported and/or fixed), they should educate the users of the ways to file helpful feedback for clang-tidy and give them tools to gather relevant diagnostic information in a form convenient for clang-tidy contributors to reproduce and fix issues. In particular, it should be easy to get the clang-tidy command line out of the IDE. Users should also be prepared to create isolated test cases from their code.

It seems to me that resolving this issue can be beneficial to both parties, improving overall user experience.

Users should understand the nature of the project. Individuals and companies, small and large, are working on the LLVM project. However, they have their own needs and priorities and are not able to provide reliable and consistent support for all use cases. While many, if not all, of us would be glad, if clang-tidy was a pure joy to use, we're limited in the time and effort we can put into resolving user-reported issues. Thus, filing feedback in a helpful form is the best way to get your issue resolved. As with almost any software product, to help you we need a precise description of the issue and an easy way to reliably reproduce it. Specifically, a self-contained source file, a command line (independent of your machine's specifics, e.g. local paths) to run clang-tidy, clang-tidy output and your expectations (and what exactly is the difference between your expectations and reality). And even with ideal feedback there's no guarantee that someone will have find time to fix the problem (though we'll try).

This should encourage improving communication between the development teams.

If CLion team has enough bandwidth to translate user-reported issues to us, we would definitely welcome this. Otherwise, they should set proper expectations and educate their users to work with the LLVM community to get their issues resolved. I don't think it's reasonable to expect the reverse (i.e. clang-tidy contributors proactively communicating with CLion developers to resolve issues CLion users run into).

BTW, contributions are also welcome ;)

llvmbot commented 4 years ago

The report I filed with CLion was closed as a 3-rd party problem. You claim that your tool does have the facility to control this issue. I will get back to the CLion team and tell them that.

That is not the consensus. CLion should make sure its passing the correct language version to clang-tidy, likewise some clang-tidy checks should be modified to ensure they only run on a supported language version.

I have put up a few reviews https://reviews.llvm.org/D75289 and (child)https://reviews.llvm.org/D75340 to help with the clang-tidy side of things, but any bug reports of checks ran on incompatible language versions would be greatly appreciated.

llvmbot commented 4 years ago

The only conclusion I get from this discussion is that there is a break in communication between the JetBrains team developing CLion, and the team developing clang-tidy. As a user of CLion I do not run the clang-tidy command myself, and I have no idea what the settings or flags are, nor how to change them. My cmake projects explicitly set the C++ standard to use, and everything else Should Just Work.

The report I filed with CLion was closed as a 3-rd party problem. You claim that your tool does have the facility to control this issue. I will get back to the CLion team and tell them that.

It seems to me that resolving this issue can be beneficial to both parties, improving overall user experience. This should encourage improving communication between the development teams.

llvmbot commented 4 years ago

File bugs for each check that is running in an incompatible version. Checks can be tailored against the version of c++(or other languages) and should disable themselves if running in an incompatible language version.

llvmbot commented 4 years ago

And make sure your compilations database contains correct -std=c++XX options.

llvmbot commented 4 years ago

Wait, the explicit keyword for constructors is a C++98 feature. You probably mean suggestions to add explicit to conversion operators (this is a C++11 feature).

Apart from that most checks should have checks for the appropriate standard version. If not, please file separate bugs with an isolated test case (ideally, without any #includes) and a full clang-tidy command line with all compiler options (e.g. clang-tidy file.cc -checks=-*,google-explicit-constructor -- -std=c++98). Each case should be handled separately.

llvmbot commented 4 years ago

Each check has its own logic re: supported language standard. If you see results from checks that are irrelevant for the language version your project uses, it's best to turn these checks off.

We can add safeguards for each check (e.g. specifically for google-explicit-constructor, one would need to change CPlusPlus to CPlusPlus11 here: https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp#L24). However, it just doesn't make sense to turn the check on for your project.

BTW, how do you choose the set of checks to run?

llvmbot commented 4 years ago

I am developing a C++ project targeting gcc 4.3 on SLES 11, which supports -std=c++03 at best. I use CLion and get clang-tidy inspection assistance.

In the following example I get some useless advice:

include

class Foo { public: Foo(std::string name) : m_name(name) {} virtual ~Foo() {}

protected: std::string m_name; };

  1. Use 'explicit' for the constructor.
  2. use '= default' for the destructor.
  3. use std::move for initializing the m_name member.
  4. If I later derive from this class, I am advised to use 'override' on the destructor.

The CLion folks tell me that clang-tidy has no way to tailor advice based on the C++ standard of the project, which they do parse out of the CMakeLists.txt file.

davidbolvansky commented 4 years ago

Real example?