Re: [PATCH 09/14] Allow nested implications for extensions.

[Jeff Law <jeffreyalaw@gmail.com>]

On 6/30/23 23:22, Christoph Müllner wrote:
> On Sat, Jul 1, 2023 at 12:24 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 6/30/23 15:53, Christoph Müllner wrote:
>>> On Fri, Jun 30, 2023 at 10:45 PM Jeff Law via Binutils
>>> <binutils@sourceware.org> wrote:
>>>>
>>>>
>>>>
>>>> On 6/29/23 11:18, Nathan Huckleberry via Binutils wrote:
>>>>> Certain extensions require two levels of implications.  For example,
>>>>> zvkng implies zvkn and zvkn implies zvkned.  Enabling zvkng should also
>>>>> enable zvkned.
>>>>>
>>>>> This patch fixes this behavior.
>>>>>
>>>>> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
>>>>> ---
>>>>>     bfd/elfxx-riscv.c | 16 ++++++++++++++--
>>>>>     1 file changed, 14 insertions(+), 2 deletions(-)
>>>> No problem with the actual code.  I would suggest a comment explicitly
>>>> stating this only allows two levels rather than arbitrary levels of nesting.
>>>
>>> If I read the resulting code correctly, then arbitrary nesting should
>>> be possible.
>>> Whenever an implicit subset is found and added, `finished` will be set to false,
>>> `t` will be reset to the initial value and the inner loop will terminate.
>>> The outer loop will start over because `finished` is false.
>>> Do I miss something?
>> This is making my brain hurt, too late on a Friday afternoon to think.
>> I should just put it under the debugger (the downside of trying to
>> review code for a hands-on learner), but I don't have the time right
>> now.  ISTM there needs to be a worklist of things we've found that we
>> then need to scan.
>>
>> But I'll trust you on this, you're a lot more familiar with this code in
>> general than I am.
>>
>> I wouldn't lose any sleep if you fixed formatting in this code.  We're
>> supposed to be using GNU style.  So the open curley on the IF statement
>> should be on its own line, indented twice (which implies the code in the
>> TRUE arm ought to be reindented) and the close curley should line up
>> with the open curley.
> 
> I tested all patches with check_GNU_style.sh and it only found
> what you have commented here.
> 
> Since nobody has pushed this series so far, I have updated the code
> and sent out a v6
> which can also be found here:
>    https://github.com/cmuellner/binutils-gdb/tree/riscv-zvk-v6
Thank you!  That'll be so much easier than applying things by hand! 
Fetching that remote now ;-)

jeff