Use of conditionals with omitted operands in amdgpu (x? : y) (was: [PATCH 4/5] dpm/amd/pm: Sienna: Remove 0 MHz as a current clock frequency (v3))

[Luben Tuikov]

+AlexD
+ChrisianK
+LKML

On 2021-10-19 03:44, Paul Menzel wrote:
> Dear Luben,
>
>
> Am 19.10.21 um 06:50 schrieb Luben Tuikov:
>> On 2021-10-19 00:38, Lazar, Lijo wrote:
>>> On 10/19/2021 9:45 AM, Luben Tuikov wrote:
>>>> On 2021-10-18 23:38, Lazar, Lijo wrote:
>>>>> On 10/19/2021 5:19 AM, Luben Tuikov wrote:
> […]
>
>>>>>> -			if (ret)
>>>>>> -				goto print_clk_out;
>>>>>> +			freq_value[1] = curr_value ?: freq_value[0];
>>>>> Omitting second expression is not standard C -
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FConditionals.html&data=04%7C01%7Cluben.tuikov%40amd.com%7Ca0515ae37fc64695640408d992d44452%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637702263078635669%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=cPRhR4Fcns4Kx%2BjkAjvN16xDk0jDEbkCO0EooJzEUlA%3D&reserved=0
>>>> Lijo just clarified to me that:
>>>>
>>>>> well, i had to look up as I haven't seen it before
>>>> I hope the following should make it clear about its usage:
>>>>
>>>> $cd linux/
>>>> $find . -name "*.[ch]" -exec grep -E "\?:" \{\} \+ | wc -l
>>>> 1042
>>>> $_
>      $ git grep -E "\?:" -- '*amdgpu*.[ch]'
>      drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c: * Solution?:
>
> So for the AMDGPU subsystem, as the only result is a comment, currently, 
> conditionals with omitted operands are not used. So, it’s a valid 
> question, if the use should be introduced into the subsystem.
>
> The GCC documentation also states:
>
>> In this simple case, the ability to omit the middle operand is not
>> especially useful. When it becomes useful is when the first operand
>> does, or may (if it is a macro argument), contain a side effect. Then
>> repeating the operand in the middle would perform the side effect
>> twice. Omitting the middle operand uses the value already computed
>> without the undesirable effects of recomputing it.
> So, in your case, there are no side effect, if I am not mistaken.

The explanation you quoted above makes a case *for* using the extension. It's telling you that it is a good thing to use the extension so should the first operand be a macro with an argument, it'll avoid being evaluated twice.

>
> I do not care, if the extension is going to be used or not.

So then why post this message, if you don't care? Since your email continues after this, like this:

> The 
> maintainers might want to officially confirm the use in the subsystem, 

I've added the maintainers, Alex and Christian, as well as LKML to the To: list of this email.
I believe that it is perfectly fine to use the ternary conditional abbreviation "c = a ?: b;", as the kernel uses it extensively, over 1000 occurrences in the kernel. It also eliminates side effects should 'a' be a macro with an argument which evaluates.

> as using these extensions is surprising for some C developers not 
> knowing the GNU extensions.
>
>>> Thanks Luben!
>> You're welcome. I'm glad you're learning new things from my patches.
>> Would've been easier if you'd just said in your email that you've
>> never seen this ternary conditional shortcut before and that you've
>> just learned of it from my patch. (Or not post anything at all in
>> this very case and get in touch with me privately via email or
>> Teams--I would've gladly clarified it there.)
> In my opinion, asking this on the list is perfectly valid, as other 
> readers, might have the same question. But being more elaborate to avoid 
> misunderstandings is always a good thing.

Lijo wasn't asking anything. There was no question in any of his emails on this thread which is all about the use of "?:", which is a well-established practice.

Why are we having a thread about the use of "?:" ?

Regards,
Luben

>
>> I hope the find+egrep above is also edifying, so you can use it in
>> the future in your learning process.
> I hope, you like my solution without using find. ;-)
>
>
> Kind regards,
>
> Paul