Replies: 6 comments 18 replies
-
|
TODO for Bob: short tutorial of how to run clang tidy to extract the actual instances associated with one of the errors |
Beta Was this translation helpful? Give feedback.
-
|
tldr. The |
Beta Was this translation helpful? Give feedback.
-
|
If you want to run all the enabled checks then type
Edit: See below for correct info |
Beta Was this translation helpful? Give feedback.
-
|
The above info isn't quite correct. I've added clang-tidy documentation to the Making a Pull Request section of the wiki. It includes how to run specific checks and how to run on specific files |
Beta Was this translation helpful? Give feedback.
-
|
The above list, reformatted as a task list.
|
Beta Was this translation helpful? Give feedback.
-
|
What I envision is that we can use this list to check off things that have been looked at, though ideally it would let someone else add comments since letting only 1 person comment is not great. I do think a linear format for this makes sense since it is probably easier to look at the clang-tidy issues at the bottom of the list first with fewer violations. |
Beta Was this translation helpful? Give feedback.
-
|
I just checked and it looks like Maintainers can edit comments so you can edit the above list, I believe. |
Beta Was this translation helpful? Give feedback.
-
|
This looks great. Then people can grab what they want, create a card, and work on it. If a particular check needs more discussion it can can be added as it's own thread in this Discussion and we can discuss it. |
Beta Was this translation helpful? Give feedback.
-
|
Output from the python script I made to parse clang-tidy output [cert-dcl50-cpp] 1 [clang-analyzer-valist.Uninitialized] 1 [hicpp-use-emplace 1 modernize-use-emplace] 1 [clang-analyzer-core.uninitialized.Assign] 1 [modernize-use-bool-literals] 1 [bugprone-signed-char-misuse 1 cert-str34-c] 1 [clang-analyzer-optin.performance.Padding] 1 [google-readability-namespace-comments 1 llvm-namespace-comment] 1 [modernize-loop-convert] 2 [google-build-using-namespace] 2 [bugprone-branch-clone] 2 [hicpp-multiway-paths-covered] 2 [bugprone-integer-division] 2 [bugprone-assignment-in-if-condition] 2 [performance-faster-string-find] 2 [modernize-use-default-member-init] 2 [performance-inefficient-vector-operation] 3 [readability-simplify-boolean-expr] 3 [cert-msc32-c 3 cert-msc51-cpp] 3 [performance-for-range-copy] 3 [cert-env33-c] 3 [bugprone-string-integer-assignment] 3 [hicpp-use-auto 4 modernize-use-auto] 4 [cppcoreguidelines-virtual-class-destructor] 4 [readability-const-return-type] 6 [cert-err58-cpp] 7 [clang-diagnostic-unused-command-line-argument] 7 [clang-analyzer-core.UndefinedBinaryOperatorResult] 8 [clang-analyzer-core.CallAndMessage] 10 [readability-convert-member-functions-to-static] 11 [performance-unnecessary-value-param] 11 [misc-confusable-identifiers] 11 [cppcoreguidelines-pro-bounds-constant-array-index] 12 [hicpp-use-noexcept 13 modernize-use-noexcept] 13 |
Beta Was this translation helpful? Give feedback.
-
import os
import sys
number_of_violations_to_print = 40
counter = {}
def process(filename):
with open(filename,'r') as ofile:
data = ofile.readlines()
for line in data:
if 'warning:' in line:
ls = line.split()
code_file_path = ls[0]
code_file = code_file_path.split('/')[-1]
violation_string = ls[-1]
violations = violation_string.split(',')
for violation in violations:
if violation not in counter:
counter[violation] = {}
if code_file not in counter[violation]:
counter[violation][code_file] = 0
counter[violation][code_file] += 1
process('tidy_results_cpp.log')
process('tidy_results_c.log')
process('tidy_results_gpu.log')
def get_total_count(key):
return sum(counter[key].values())
vios = sorted(list(counter.keys()),key=get_total_count)
for i in range(number_of_violations_to_print):
print()
print(vios[i],get_total_count(vios[i]))
for key in counter[vios[i]]:
print(key,counter[vios[i]][key]) |
Beta Was this translation helpful? Give feedback.
-
|
About @evaneschneider and @alwinm, I'd like your input on this. |
Beta Was this translation helpful? Give feedback.
-
|
I agree. I don't think it's used anywhere. I'm fine with deleting that source file and the associated calls in mpi_routines.cpp and simplifying the makefile. |
Beta Was this translation helpful? Give feedback.
-
|
This is now documented in Issue #253 |
Beta Was this translation helpful? Give feedback.
-
|
The last two
For both of these the bulk of the errors are because we use |
Beta Was this translation helpful? Give feedback.
-
I am not sure I understand this sentence but I do believe you understand what I am conveying. Sometimes you need a signed number and ptrdiff_t is that number is all I'm trying to say and I do feel heard. The second part of the sentence confuses me because if you use ptrdiff_t instead of size_t where you need to, that prevents bugs tautologically because of the word "need." I think at this point we agree on the considerations that need to be taken into account for making this change and we can agree to disagree on the value/effort ratio. |
Beta Was this translation helpful? Give feedback.
-
|
I think the difference is that I’m not saying we should index with ptrdiff_t, just use it to compute the appropriate size_t variable |
Beta Was this translation helpful? Give feedback.
-
|
Thank you for the clarification. If putting (size_t) in every index will fix both implicit widening and narrowing conversions without needing to change other variables currently declared as I am mostly against changing variables currently explicitly declared as int to size_t except when the appropriate thought and consideration has been taken, and if necessary to use ptrdiff_t. But for this latter point I don't think you were proposing that, I felt I should just add a warning saying why we shouldn't do a blanket change from int to size_t and describing some possible traps along the way depending on the choices that are made. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, 100% not all I think I'll put this on the back burner for now, for the same effort I think we can go through more checks and probably have a greater impact. If fancy new GPUs with like 500GB of memory come out we can make this a priority then. |
Beta Was this translation helpful? Give feedback.
-
|
Per a discussion with @evaneschneider offline I'm converting this into an issue as it would probably be a good one for a new student or collaborator. |
Beta Was this translation helpful? Give feedback.
-
First column is total warnings, second column is percent of total failures, third column is the check
Glossary of checks in clang-tidy 15
Beta Was this translation helpful? Give feedback.
All reactions