Add clang-tidy, clean up some warnings #71
Conversation
* Add initial .clang-tidy config * Clear code base of found lints
| const size_t * buffer_origin, | ||
| const size_t * host_origin, | ||
| const size_t * buffer_offset, | ||
| const size_t * host_offset, |
There was a problem hiding this comment.
These are called buffer_origin and host_origin in the spec, but I've just looked the the headers and see they are *_offset there. It's kind of beyond the scope of this PR, but they should really all be the same. *_origin makes more sense to me, but it doesn't make much difference.
There was a problem hiding this comment.
Are you suggesting to open a PR on the headers to change the naming of this parameter (and the others you commented on)?
There was a problem hiding this comment.
I just had another thought: I put readability-inconsistent-declaration-parameter-name into the WarningsAsErrors list.
Do we want to support slightly mismatching Header/ICD versions? Because putting it there would break that.
| const size_t * host_origin, | ||
| cl_bool blocking_write, | ||
| const size_t * buffer_offset, | ||
| const size_t * host_offset, |
There was a problem hiding this comment.
Ditto comment re: origin vs offset.
The blocking_read to blocking_write of course makes sense.
| { | ||
| KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(function_name, NULL); | ||
| KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(func_name, NULL); |
There was a problem hiding this comment.
In the spec this one is funcname, but func_name from the header seems better, I can change that in the spec.
test/driver_stub/cl.c
Outdated
| @@ -863,7 +863,7 @@ clLinkProgram(cl_context context , | |||
| void * user_data , | |||
| cl_int * errcode_ret) CL_API_SUFFIX__VERSION_1_2 | |||
| { | |||
| cl_program obj = (cl_program) malloc(sizeof(cl_program)); | |||
| cl_program obj = (cl_program) malloc(sizeof(*obj)); | |||
There was a problem hiding this comment.
Changing this to malloc(sizeof(struct _cl_program)) seems easier to read to me.
There was a problem hiding this comment.
Well, I disagree :P
Not that it's likely, but this sidesteps the typedef, so changing the typedef of cl_program would reintroduce the very same bug again. It'd rather DRY and state the type exactly once.
Speaking of DRY, I should remove the cast here.
It does not fully resolve #68 since we still need to enable a higher warning level in the compiler itself.
I've enabled all checks in clang-tidy and marked a few ones that I encountered while fixing the code as errors.
misc-unused-parametersbreaks on C, that's why I disabled it. We can change the concrete list of checks later on and tweak some parameters, but this PR cleans the code of all things found so far by clang-tidy.The
mallocof wrong size in one of the tests was a really good catch.That's probably as good as we can get without converting the project to C++ (which, I mean, why not...)