Implemented Clang Tidy #621
Conversation
…s directly in the source folder that are not auto generated.
|
I don't think this will be very successful unless scripts are removed, please let me know your opinion on that, and if you want to keep the scripts, I will close this PR. |
…utilities.hpp header to avoid needless dozens of includes.
… is a utility to interface with half.
…estions for IWYU.
…y headers that it is necessary.
…fixed but it might not be.
…he library works with MSVC and android.
CNugteren
left a comment
CNugteren
left a comment
There was a problem hiding this comment.
This PR makes a lot of changes. Not sure if I can properly review all of it. Is there a way to split this up per type of change perhaps? Or would that be a lot of work for you?
I just went through 10 out of the 150 files and gave some comments.
| const auto device_cpp = Device(device); | ||
| const auto platform_id = device_cpp.PlatformID(); | ||
| const auto device_name = GetDeviceName(device_cpp); | ||
| auto* const platform_id = device_cpp.PlatformID(); |
There was a problem hiding this comment.
For clarity, it is suggested to use auto* const instead of const auto which has the same effect. Found throughout the code.
There was a problem hiding this comment.
I read https://www.nuonsoft.com/blog/2018/07/06/const-auto-versus-const-auto-for-pointer-types/ and I don't think it has the same effect. But it seems good to do indeed if there is a pointer involved.
However, now I need to go through all these changes and check if indeed a pointer is involved and if indeed the pointed-to object is const (rather than the pointer itself). Will be quite some work... Starting with this particular one, why is this a pointer? The function returns a cl_device_id, which OpenCL might have defined as a pointer to something, but it is not entirely clear.
I would suggest to revert all these auto* const changes to make reviewing a lot easier. That way we can have this clang-tidy PR merged in sooner. Later we could go case-by-case and see if we can improve these things, but it is also not that important I would say.
| if ((db.kernel == this_kernel) && (db.precision == this_precision || db.precision == Precision::kAny)) { | ||
| // Searches for the right vendor and device type, or selects the default if unavailable | ||
| const auto parameters = | ||
| auto parameters = |
There was a problem hiding this comment.
Const correctness prevents a move, thus potentially preventing compiler optimization.
There was a problem hiding this comment.
Can you share a link to the clang-tidy check about this one? Thanks.
There was a problem hiding this comment.
| use_faster_kernel && IsMultiple(n, getDatabase()["WGS"] * getDatabase()["WPT"] * getDatabase()["VW"]); | ||
|
|
||
| // If possible, run the fast-version of the kernel | ||
| const auto kernel_name = (use_fastest_kernel) ? "XaxpyFastest" : (use_faster_kernel) ? "XaxpyFaster" : "Xaxpy"; |
There was a problem hiding this comment.
For clarity it’s recommended to use if instead of nesting multiple ?, you’ll see this in a few places that pick different kernels.
There was a problem hiding this comment.
In this case I find the original much clearer, please revert.
|
There is no easy way to simplify the readability of these changes, however, I will point out that most of these are not functional changes. They simply either improve code readability or performance with minimal actual changes. I would say most of the actual changes are regarding the preprocessor file or files that use multiple kernels, because the ternary operator has been replaced with if statements for 2 or more ternary operators. |
|
I have to fix IWYU on this PR, so can't be merged yet. |
Thanks, I understand. Yeah indeed ideally most of your changes could likely be blindly accepted. Still, we have seen in other linting PRs in the past (e.g. IWYU) that even those changes can break functionality. So I'm a bit afraid of simply pressing the accept button here, especially after seeing the first few files and already noticing a few issues. I understand that the tests should cover most of the correctness, but speed is another issue that can't be easily checked. And then there is code-style, with which I mean that blindly applying a linter might not always be the best idea. I also can't go and review all 37,221 additions and 25,945 removals. Or better said, I could review them, but then it has to be worth my time. So I'm not really sure what to do with this PR yet. What do you think? One option could be to make a new PR where you add clang-tidy checks to CI with the most minimal amount of checks possible (and thus only a very limited set of fixes). Ideally each commit should be one type of change, such that it is easy to review. And then once we merge that in we might add a second PR with a few more checks? I understand that is work for you of course, so feel free to disagree. |
|
I think it would be fastest to do multiple PRs, so I’ll leave this as reference for now, but create mini PR’s to build up to this. |
2 participants
I went through and did a lot of clang tidy checks to start integrating this codebase with all of modern features.