Skip to content

Add clang-tidy, clean up some warnings #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mmha wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add clang-tidy, clean up some warnings #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe99052
Add clang-tidy
mmha May 12, 2019
9f605dd
Add comments on NOLINT
mmha May 13, 2019
931f167
Remove unnecessary pointer cast
mmha May 13, 2019
8fab9df
Whitelist warning categories
mmha May 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
Checks: 'bugprone-*,misc-*,readability-*,performance-*,-misc-unused-parameters'
WarningsAsErrors: 'readability-inconsistent-declaration-parameter-name,hicpp-braces-around-statements,bugprone-macro-parentheses,bugprone-suspicious-string-compare'
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle: file
CheckOptions:
- key: readability-inconsistent-declaration-parameter-name.Strict
value: 1
...

6 changes: 4 additions & 2 deletions loader/icd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,10 @@ void khrIcdVendorAdd(const char *libraryName)

// add this vendor to the list of vendors at the tail
{
KHRicdVendor **prevNextPointer = NULL;
for (prevNextPointer = &khrIcdVendors; *prevNextPointer; prevNextPointer = &( (*prevNextPointer)->next) );
KHRicdVendor **prevNextPointer = &khrIcdVendors;
while(*prevNextPointer) {
prevNextPointer = &( (*prevNextPointer)->next);
}
*prevNextPointer = vendor;
}

Expand Down
84 changes: 42 additions & 42 deletions loader/icd_dispatch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,17 @@ clGetDeviceInfo(
CL_API_ENTRY cl_int CL_API_CALL
clCreateSubDevices(cl_device_id in_device,
const cl_device_partition_property * properties,
cl_uint num_entries,
cl_uint num_devices,
cl_device_id * out_devices,
cl_uint * num_devices) CL_API_SUFFIX__VERSION_1_2
cl_uint * num_devices_ret) CL_API_SUFFIX__VERSION_1_2
{
KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(in_device, CL_INVALID_DEVICE);
return in_device->dispatch->clCreateSubDevices(
in_device,
properties,
num_entries,
num_devices,
out_devices,
num_devices);
num_devices_ret);
}

CL_API_ENTRY cl_int CL_API_CALL
Expand Down Expand Up @@ -823,7 +823,7 @@ clEnqueueReadBuffer(cl_command_queue command_queue,
cl_mem buffer,
cl_bool blocking_read,
size_t offset,
size_t cb,
size_t size,
void * ptr,
cl_uint num_events_in_wait_list,
const cl_event * event_wait_list,
Expand All @@ -835,7 +835,7 @@ clEnqueueReadBuffer(cl_command_queue command_queue,
buffer,
blocking_read,
offset,
cb,
size,
ptr,
num_events_in_wait_list,
event_wait_list,
Expand All @@ -847,8 +847,8 @@ clEnqueueReadBufferRect(
cl_command_queue command_queue,
cl_mem buffer,
cl_bool blocking_read,
const size_t * buffer_origin,
const size_t * host_origin,
const size_t * buffer_offset,
const size_t * host_offset,
Copy link
Contributor

@alycm alycm May 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mmha mmha May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting to open a PR on the headers to change the naming of this parameter (and the others you commented on)?

Copy link
Contributor Author

@mmha mmha May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 * region,
size_t buffer_row_pitch,
size_t buffer_slice_pitch,
Expand All @@ -864,8 +864,8 @@ clEnqueueReadBufferRect(
command_queue,
buffer,
blocking_read,
buffer_origin,
host_origin,
buffer_offset,
host_offset,
region,
buffer_row_pitch,
buffer_slice_pitch,
Expand All @@ -882,7 +882,7 @@ clEnqueueWriteBuffer(cl_command_queue command_queue,
cl_mem buffer,
cl_bool blocking_write,
size_t offset,
size_t cb,
size_t size,
const void * ptr,
cl_uint num_events_in_wait_list,
const cl_event * event_wait_list,
Expand All @@ -894,7 +894,7 @@ clEnqueueWriteBuffer(cl_command_queue command_queue,
buffer,
blocking_write,
offset,
cb,
size,
ptr,
num_events_in_wait_list,
event_wait_list,
Expand All @@ -905,9 +905,9 @@ CL_API_ENTRY cl_int CL_API_CALL
clEnqueueWriteBufferRect(
cl_command_queue command_queue,
cl_mem buffer,
cl_bool blocking_read,
const size_t * buffer_origin,
const size_t * host_origin,
cl_bool blocking_write,
const size_t * buffer_offset,
const size_t * host_offset,
Copy link
Contributor

@alycm alycm May 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto comment re: origin vs offset.

The blocking_read to blocking_write of course makes sense.

const size_t * region,
size_t buffer_row_pitch,
size_t buffer_slice_pitch,
Expand All @@ -922,9 +922,9 @@ clEnqueueWriteBufferRect(
return command_queue->dispatch->clEnqueueWriteBufferRect(
command_queue,
buffer,
blocking_read,
buffer_origin,
host_origin,
blocking_write,
buffer_offset,
host_offset,
region,
buffer_row_pitch,
buffer_slice_pitch,
Expand All @@ -942,7 +942,7 @@ clEnqueueFillBuffer(cl_command_queue command_queue,
const void * pattern,
size_t pattern_size,
size_t offset,
size_t cb,
size_t size,
cl_uint num_events_in_wait_list,
const cl_event * event_wait_list,
cl_event * event) CL_API_SUFFIX__VERSION_1_2
Expand All @@ -954,7 +954,7 @@ clEnqueueFillBuffer(cl_command_queue command_queue,
pattern,
pattern_size,
offset,
cb,
size,
num_events_in_wait_list,
event_wait_list,
event);
Expand All @@ -966,7 +966,7 @@ clEnqueueCopyBuffer(cl_command_queue command_queue,
cl_mem dst_buffer,
size_t src_offset,
size_t dst_offset,
size_t cb,
size_t size,
cl_uint num_events_in_wait_list,
const cl_event * event_wait_list,
cl_event * event) CL_API_SUFFIX__VERSION_1_0
Expand All @@ -978,7 +978,7 @@ clEnqueueCopyBuffer(cl_command_queue command_queue,
dst_buffer,
src_offset,
dst_offset,
cb,
size,
num_events_in_wait_list,
event_wait_list,
event);
Expand Down Expand Up @@ -1173,7 +1173,7 @@ clEnqueueMapBuffer(cl_command_queue command_queue,
cl_bool blocking_map,
cl_map_flags map_flags,
size_t offset,
size_t cb,
size_t size,
cl_uint num_events_in_wait_list,
const cl_event * event_wait_list,
cl_event * event,
Expand All @@ -1186,7 +1186,7 @@ clEnqueueMapBuffer(cl_command_queue command_queue,
blocking_map,
map_flags,
offset,
cb,
size,
num_events_in_wait_list,
event_wait_list,
event,
Expand Down Expand Up @@ -1357,9 +1357,9 @@ clEnqueueBarrierWithWaitList(cl_command_queue command_queue,

CL_API_ENTRY void * CL_API_CALL
clGetExtensionFunctionAddressForPlatform(cl_platform_id platform,
const char * function_name) CL_API_SUFFIX__VERSION_1_2
const char * func_name) CL_API_SUFFIX__VERSION_1_2
{
KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(function_name, NULL);
KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(func_name, NULL);
Copy link
Contributor

@alycm alycm May 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spec this one is funcname, but func_name from the header seems better, I can change that in the spec.


// make sure the ICD is initialized
khrIcdInitialize();
Expand All @@ -1370,7 +1370,7 @@ clGetExtensionFunctionAddressForPlatform(cl_platform_id platform,
// do not need to be ICD-aware and do not require any ICD loader
// modifications. The KHR and EXT extensions below were added for
// backwards compatibility only.
#define CL_COMMON_EXTENSION_ENTRYPOINT_ADD(name) if (!strcmp(function_name, #name) ) return (void *)(size_t)&name
#define CL_COMMON_EXTENSION_ENTRYPOINT_ADD(name) if (!strcmp(func_name, #name) ) return ((void *)(size_t)&(name))

// Functions supporting the creation of OpenCL Memory Objects
// from OpenGL Objects (cl_apple_gl_sharing, cl_khr_gl_sharing)
Expand Down Expand Up @@ -1436,7 +1436,7 @@ clGetExtensionFunctionAddressForPlatform(cl_platform_id platform,
KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(platform, NULL);
return platform->dispatch->clGetExtensionFunctionAddressForPlatform(
platform,
function_name);
func_name);
}

// Deprecated APIs
Expand All @@ -1457,15 +1457,15 @@ clSetCommandQueueProperty(cl_command_queue command_queue,
CL_API_ENTRY cl_int CL_API_CALL
clCreateSubDevicesEXT(
cl_device_id in_device,
const cl_device_partition_property_ext * partition_properties,
const cl_device_partition_property_ext * properties,
cl_uint num_entries,
cl_device_id * out_devices,
cl_uint * num_devices) CL_EXT_SUFFIX__VERSION_1_1_DEPRECATED
{
KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(in_device, CL_INVALID_DEVICE);
return in_device->dispatch->clCreateSubDevicesEXT(
in_device,
partition_properties,
properties,
num_entries,
out_devices,
num_devices);
Expand Down Expand Up @@ -1569,24 +1569,24 @@ clEnqueueBarrier(cl_command_queue command_queue) CL_EXT_SUFFIX__VERSION_1_1_DEPR
}

CL_API_ENTRY void * CL_API_CALL
clGetExtensionFunctionAddress(const char *function_name) CL_EXT_SUFFIX__VERSION_1_1_DEPRECATED
clGetExtensionFunctionAddress(const char *func_name) CL_EXT_SUFFIX__VERSION_1_1_DEPRECATED
{
size_t function_name_length = 0;
KHRicdVendor* vendor = NULL;

KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(function_name, NULL);
KHR_ICD_VALIDATE_HANDLE_RETURN_ERROR(func_name, NULL);

// make sure the ICD is initialized
khrIcdInitialize();
function_name_length = strlen(function_name);
function_name_length = strlen(func_name);

// return any ICD-aware extensions

// Most extensions, including multi-vendor KHR and EXT extensions,
// do not need to be ICD-aware and do not require any ICD loader
// modifications. The KHR and EXT extensions below were added for
// backwards compatibility only.
#define CL_COMMON_EXTENSION_ENTRYPOINT_ADD(name) if (!strcmp(function_name, #name) ) return (void *)(size_t)&name
#define CL_COMMON_EXTENSION_ENTRYPOINT_ADD(name) if (!strcmp(func_name, #name) ) return ((void *)(size_t)&(name))

// Functions supporting the creation of OpenCL Memory Objects
// from OpenGL Objects (cl_apple_gl_sharing, cl_khr_gl_sharing)
Expand Down Expand Up @@ -1652,10 +1652,10 @@ clGetExtensionFunctionAddress(const char *function_name) CL_EXT_SUFFIX__VERSION_
size_t vendor_suffix_length = strlen(vendor->suffix);
if (vendor_suffix_length <= function_name_length && vendor_suffix_length > 0)
{
const char *function_suffix = function_name+function_name_length-vendor_suffix_length;
const char *function_suffix = func_name+function_name_length-vendor_suffix_length;
if (!strcmp(function_suffix, vendor->suffix) )
{
return vendor->clGetExtensionFunctionAddress(function_name);
return vendor->clGetExtensionFunctionAddress(func_name);
}
}
}
Expand Down Expand Up @@ -1835,13 +1835,13 @@ CL_API_ENTRY cl_int CL_API_CALL clGetGLContextInfoKHR(

CL_API_ENTRY cl_event CL_API_CALL clCreateEventFromGLsyncKHR(
cl_context context,
cl_GLsync sync,
cl_GLsync cl_GLsync,
cl_int * errcode_ret) CL_API_SUFFIX__VERSION_1_1
{
KHR_ICD_VALIDATE_HANDLE_RETURN_HANDLE(context, CL_INVALID_CONTEXT);
return context->dispatch->clCreateEventFromGLsyncKHR(
context,
sync,
cl_GLsync,
errcode_ret);
}

Expand Down Expand Up @@ -2232,17 +2232,17 @@ clSetUserEventStatus(
CL_API_ENTRY cl_mem CL_API_CALL
clCreateFromEGLImageKHR(
cl_context context,
CLeglDisplayKHR display,
CLeglImageKHR image,
CLeglDisplayKHR egldisplay,
CLeglImageKHR eglimage,
cl_mem_flags flags,
const cl_egl_image_properties_khr *properties,
cl_int *errcode_ret)
{
KHR_ICD_VALIDATE_HANDLE_RETURN_HANDLE(context, CL_INVALID_CONTEXT);
return context->dispatch->clCreateFromEGLImageKHR(
context,
display,
image,
egldisplay,
eglimage,
flags,
properties,
errcode_ret);
Expand Down
13 changes: 8 additions & 5 deletions loader/linux/icd_linux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@
*/

#include "icd.h"
#include <dirent.h>
#include <dlfcn.h>
#include <pthread.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <dirent.h>
#include <pthread.h>

static pthread_once_t initialized = PTHREAD_ONCE_INIT;

Expand Down Expand Up @@ -72,7 +72,7 @@ void khrIcdOsVendorsEnumerate(void)
{
break;
}
if (strcmp(dirEntry->d_name + strlen(dirEntry->d_name) - strlen(extension), extension) )
if (strcmp(dirEntry->d_name + strlen(dirEntry->d_name) - strlen(extension), extension) != 0)
{
break;
}
Expand Down Expand Up @@ -113,7 +113,10 @@ void khrIcdOsVendorsEnumerate(void)
break;
}
// ignore a newline at the end of the file
if (buffer[bufferSize-1] == '\n') buffer[bufferSize-1] = '\0';
if (buffer[bufferSize-1] == '\n')
{
buffer[bufferSize-1] = '\0';
}

// load the string read from the file
khrIcdVendorAdd(buffer);
Expand Down
Loading