Re: [patch PE-COFF]: Some minor nits

[Kai Tietz <ktietz70 at googlemail dot com>]

2009/3/18 Dave Korn <dave.korn.cygwin@googlemail.com>:
> Kai Tietz wrote:
>
>> Thanks Dave for reviewing it. Here the adjusted patch as discussed.
>
> ?Oh, sorry to be such a nuisance, I've just spotted another potential hiccup,
> only minor though:
>
> @@ -1245,6 +1244,8 @@
> ? ? sec_flags |= SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD;
> ?#endif
>
> + ?section->flags = sec_flags;
> +
> ? if (flags_ptr)
> ? ? * flags_ptr = sec_flags;
>
>
> ?This changes the behaviour of the function styp_to_sec_flags. ?As far as I
> could see the only client is this code in make_a_section_from_file in coffgen.c:
>
>
> ?return_section->lineno_count = hdr->s_nlnno;
> ?return_section->userdata = NULL;
> ?return_section->next = NULL;
> ?return_section->target_index = target_index;
>
> ?if (! bfd_coff_styp_to_sec_flags_hook (abfd, hdr, name, return_section,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? & flags))
> ? ?result = FALSE;
>
> ?return_section->flags = flags;
>
> ... so it will make no difference in this case, but I still think it isn't
> right for what should be a simple conversion function returning an integer to
> modify one of its input parameters. ?Nothing about the function name suggests
> it will modify the section object, nor the documentation comment at the top.
> I think the assignment can be safely dropped.
>
> ? ?cheers,
> ? ? ?DaveK
>

Well, you're right. The issue here was for me, that locally I removed
in the function make_a_section_from_file the assignment of
section->flags. I removed this hunk from the patch.

Cheers,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Attachment: noread.diff
Description: Binary data