YanzhaoW
commented
1 year ago @klenze
modernize-use-trailing-return-type
I suggest we should keep this feature and make the trailing return type as the default for the upcoming code with following reasons:
- readability
In my opinion, it does increase the readability, especially for the functions with long return type:
Neuland::Points Neuland::GetPoints() const;
auto Neuland::GetPoints() const -> Points;
2. Consistency
And if you use auto a lot for the variables:
```cpp
auto vec = std::vector<int>{};
const auto i = 3;
const auto points = GetPoints();I would say for the sake of consistency, it's also better to declare or define functions as:
auto myFun() -> int;
auto myFun() -> int { return 0};Trailing return type has been existed for 12 years (first introduced in C++11) and is definitely not latest trend in C++ community. Anyone who doesn't know this can learn this in few seconds. Just put the auto in the front and put the type behind the function name. It costs almost nothing to learn and only gives nothing but good in return.
cppcoreguidelines-owning-memory
We discussed that ROOT cannot allow us to have a clear ownership to the TH1 objects. So what we can do is to enable the LegacyResourceProducers option, which allows the usage of new operator but still forbids the usage of delete. If someone decides to delete some pointers, use std::unique_ptr.
Maybe we can define some factory functions to mitigate the new problem from ROOT, something like:
// instead of
auto* hist1 = new TH1D("hist1", "hist1", 10, 0,10);
//use
auto* hist1 = UseRoot<TH1D>("hist1", "hist1", 10, 0,10);A very quick thought about the implementation of UseRoot: (for C++ novice, please ignore following code block)
template<typename Type, typename ...Args>
inline auto UseRoot(Args&& ... args)
{
return new Type(std::forward<Args>(args)...); //NOLINT
}With this, we can also get rid of "new" totally in new code.
We are not a C++ shop where every contributor can be expected to stay up to date on the latest trends in C++.
The clang-tidy checking is only applied with C++17, which is 6 years ago and most of suggestions date back to 12 years ago. I know 12 years in the physics (science) community means nothing. But it is like a whole new era for (software) engineering community.
This policy was not good for clang-format, and it will be disastrous for clang-tidy
Yes, I agree. For the legacy code, there is very little we can do. Clang-tidy only checks the changed lines. So we could set the bar a little bit higher for the new committed code.
The check is bugprone rather than a matter of taste.
I would say most checks in clang-tidy are either for bugprone or readability. A matter of taste is something like east const or const west, which I also feel unnecessary to prefer either one of them.
clang-tidy probably will not check macros
It won't check whether the C macros are used correctly. It just disallows the macro usage at all with cppcoreguidelines-macro-usage, which I totally agree. See this post for many setbacks of macros. I haven't seen any case where macro cannot be replaced by other proper functions or templates.
There are still a lot to do with our CI test, like adding more compiler options and compiling against both gcc and clang. More checks are alway preferred. Also the CI test should require fresh compilation and test of R3BRoot rather than compiling on previous built binaries.
Btw, did you write the hard coded check-options in .clang-tidy one by one? And for many options, I can't even find them in clang-15 documents.
klenze
inkdot7
jose-luis-rs
ryotani
I have added a static analysis using clang-tidy into our github CI/CD test in the PR #825. The current version of clang-tidy is 15 and the check list can be found here.
This issue is dedicated to discuss what should be enabled and disabled in the file
.clang-tidy.If anyone thinks some checks are unnecessary or hard to be fixed, please make a new PR about the .clang-tidy file in the project root directory or leave a comment below.
tips:
//NOLINTbehind the line of code, clang-tidy will not check the line.~