[Mesa-dev] [Mesa-stable] [PATCH 2/6] clover: Call clBuildProgram() notification function when build completes v2

[Emil Velikov]

Hi Tom,

On 31 March 2015 at 15:29, Francisco Jerez <currojerez at riseup.net> wrote:
> Tom Stellard <thomas.stellard at amd.com> writes:
>
>> v2:
>>   - Only call notification for build errors
>>   - Fix clCompileProgram()
>>
>> Cc: 10.5 10.4 <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/gallium/state_trackers/clover/api/program.cpp | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/api/program.cpp b/src/gallium/state_trackers/clover/api/program.cpp
>> index 60184ed..5cd543c 100644
>> --- a/src/gallium/state_trackers/clover/api/program.cpp
>> +++ b/src/gallium/state_trackers/clover/api/program.cpp
>> @@ -180,12 +180,18 @@ clBuildProgram(cl_program d_prog, cl_uint num_devs,
>>     validate_build_program_common(prog, num_devs, d_devs, pfn_notify, user_data);
>>
>>     prog.build(devs, opts);
>> +   if (pfn_notify)
>> +      pfn_notify(d_prog, user_data);
>
> Maybe leave blank lines around the conditional where preceded/followed
> by another statement?
>
>>     return CL_SUCCESS;
>> +} catch (const build_error &e) {
>> +   if (pfn_notify)
>> +      pfn_notify(d_prog, user_data);
>> +   if (e.get() == CL_COMPILE_PROGRAM_FAILURE)
>> +      return CL_BUILD_PROGRAM_FAILURE;
>> +   return e.get();
>
> You can just return CL_BUILD_PROGRAM_FAILURE here unconditionally.
>
>>  } catch (error &e) {
>>     if (e.get() == CL_INVALID_COMPILER_OPTIONS)
>>        return CL_INVALID_BUILD_OPTIONS;
>> -   if (e.get() == CL_COMPILE_PROGRAM_FAILURE)
>> -      return CL_BUILD_PROGRAM_FAILURE;
>>     return e.get();
>>  }
>>
>> @@ -223,8 +229,14 @@ clCompileProgram(cl_program d_prog, cl_uint num_devs,
>>        objs<allow_empty_tag>(d_header_progs, num_headers));
>>
>>     prog.build(devs, opts, headers);
>> +   if (pfn_notify)
>> +      pfn_notify(d_prog, user_data);
>
> Same here.
>
Did you had the chance to address Francisco's comments ? Did this
patch fell through the cracks, or should I consider it
obsolete/rejected ?

Thanks
Emil