Re: [PATCH] ld: Sort section contributions in PDB files

[Mark Harmstone <mark@harmstone.com>]

On 22/2/23 10:52, Alan Modra wrote:
> On Wed, Feb 22, 2023 at 08:08:43AM +0100, Jan Beulich wrote:
>> On 22.02.2023 06:48, Alan Modra wrote:
>>> On Tue, Feb 21, 2023 at 09:26:33AM +0100, Jan Beulich wrote:
>>>> On 20.02.2023 15:13, Mark Harmstone wrote:
>>>>> +/* Used as parameter to qsort, to sort section contributions by section and
>>>>> +   offset.  */
>>>>> +static int
>>>>> +section_contribs_compare (const void *p1, const void *p2)
>>>>> +{
>>>>> +  const struct in_sc *sc1 = (const struct in_sc *) p1;
>>>>> +  const struct in_sc *sc2 = (const struct in_sc *) p2;
>>>> In ANSI C there's no need for these casts; it may be that they were
>>>> needed in pre-ANSI dialects like K&R. Personally I view _any_ cast
>>>> as latently dangerous, and hence I'd prefer if casts were used only
>>>> if there's no other option.
>>> I agree that it's fine to write this without the casts, and I've even
>>> used the cast to void* you mention later in your email to shorten
>>> lines myself.  I agree that casts are inherently dangerous too, and
>>> that it's a good idea to not use them unless necessary.  Also, it's
>>> really, really annoying to need casts because something like
>>>    os = &lang_os_list.head->output_section_statement;
>>> gets a ubsan warning when lang_os_list.head is NULL, forcing you to
>>> use a cast or accessor that loses the type checking or to write
>>> horrendous code.  There have been bugs in ld list handling due to
>>> casts.
>>>
>>> However, I'm inclined to say that a cast in a qsort comparison
>>> function, or to cast the return of malloc or similar is mostly a
>>> matter of style.  If a contributor wants to write it that way, I'm
>>> fine with approving new code with these casts.  After all, there is
>>> plenty of such code in binutils already.
>> Now that's an interesting perspective. Depending on feedback on the
>> topic I was meaning to possibly conclude that such unnecessary casts
>> would be ripe for removal. Perhaps not by hunting them down and
>> replacing them in an isolated effort, but as code is being touched
>> anyway.
> I'm fine with doing that too, particularly as you touch code.
>
> What I was trying to say, is that I think it's important for
> maintainers to allow contributors some freedom in style, to tolerate
> things that don't matter that much.
>
Thanks all, I'll resubmit with the changes suggested.

Jan, something to bear in mind is that doing such a thing would be counter-productive if binutils were ever to be converted to C++. I don't know if anybody's looked into such a thing in earnest - I'm guessing the project's too large for it to be worth anyone's time. But it'd be much more pleasant if the files in emultempl etc. were nice templated cpp files, rather than what's there at present :-)

Mark