search

cornelisnetworks / opa-psm2

Other
37 stars 29 forks source link

Remove an excessive abs for the usigned value #18

Closed dmitrygx closed 7 years ago

dmitrygx commented 7 years ago

I've submitted the issue 01org/psm#18 to the psm Does this make sense? The result is always unsigned value, there is no need to invoke abs for that

Signed-off-by: Dmitry Gladkov dmitry.gladkov@intel.com

dmitrygx commented 7 years ago

The abs() function serves dual purpose.

Agree in this case. but the I've got a compilation failure for the clang compiler:

ipath_time.c:282:9: error: taking the absolute value of unsigned type 'unsigned int' has no effect [-Werror,-Wabsolute-value]
    if (abs(new_pico_per_cycle - old_pico_per_cycle) < 5)
        ^
ipath_time.c:282:9: note: remove the call to 'abs' since unsigned values cannot be negative
    if (abs(new_pico_per_cycle - old_pico_per_cycle) < 5)

Sorry, it's part for the psm, but the same for the psm2. Can we deal with that?

dmitrygx commented 7 years ago

Btw, would something like (new_pico_per_cycle > old_pico_per_cycle) ? (new_pico_per_cycle - old_pico_per_cycle) : (old_pico_per_cycle - new_pico_per_cycle ); or max(new_pico_per_cycle , old_pico_per_cycle ) - min(new_pico_per_cycle , old_pico_per_cycle ); be more clear in this case?

bsmith94 commented 7 years ago

It seems like the expected result of the operation should be within the range of int, and a cast corrects the logic of the conditional.

if (abs((int)(new_pico_per_cycle - old_pico_per_cycle)) < 5)

Brian T. Smith System Fabric Works Senior Technical Staff bsmith@systemfabricworks.com (512) 293-4472 GPG Key: B3C2C7B73BA3CD7F

On Fri, Oct 13, 2017 at 3:06 AM, Dmitry Gladkov notifications@github.com wrote:

Btw, maybe something like (new_pico_per_cycle > old_pico_per_cycle) ? (new_pico_per_cycle - old_pico_per_cycle) : (old_pico_per_cycle - new_pico_per_cycle ); or max(new_pico_per_cycle , old_pico_per_cycle ) - min(new_pico_per_cycle , old_pico_per_cycle ); would be more clear in this case?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/01org/opa-psm2/pull/18#issuecomment-336381206, or mute the thread https://github.com/notifications/unsubscribe-auth/AAl8DWd8QqsmC_liYCuIywrwDjrCjmMjks5srxn_gaJpZM4P3rOE .

dmitrygx commented 7 years ago

This is also ok in this case

If stakeholders are fine with that, I can do the abs((int32_t)(new_pico_per_cycle - old_pico_per_cycle) < 5)