Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Tom de Vries <tdevries@suse.de>]

On 8/29/25 02:20, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Tom> +      if (addrmap_node_value (n))
> Tom> +    {
> Tom> +      /* Already mapped.  */
> Tom> +      full_range = false;
> Tom> +      continue;
> Tom> +    }
> Tom> +
> Tom> +      addrmap_node_set_value (n, obj);
> 
>>> FWIW, I still think that they way I wrote is is much better.  My
>>> opinion is based on this coding standard rule (
>>> https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
>>> ).
> 
> I don't think we should use LLVM's coding standards. 

Hi Tom,

I realize I'm probably not going to convince you, so all I'm trying to 
do here is to better make my argument, since I have not done a good job 
sofar.

I'm not advocating that we should use LLVM's coding standards, regardless.

I'm saying that we could use some of them if they make sense and don't 
conflict with the existing coding standard.

Reading the rule was a light-bulb moment for me, and has shaped my 
opinion on how to write code.

Agreed, it can be argued that the rule doesn't apply here because the 
if-else branches are too small.

It also says "Aim to reduce indentation where possible when it doesn’t 
make it more difficult to understand the code".

Of course it's highly debatable how to fill this in.

For me, it makes sense to look at the impact of the branches.

To illustrate my point, I think it makes sense to do:
...
if (...)
   a = b;
else
   c = d;
...
but not
...
if (...)
   a = b;
else
   exec ("rm -Rf /")
...

In the latter case, I'd rather write:
...
if (!...)
   {
      a = b;
      continue;
   }

   exec ("rm -Rf /");
...
to have the most impactful statement stand out both by less indentation, 
and by having it standalone rather than as part of an if/else.

And I feel the same applies to:
...
if (...)
   { /* Don't do something and make a note of it.  */ }
else
   { /* Do something. */ }
...

> Particularly so
> now that I've worked on LLVM this last year.
> 
> Simon> I also like early returns and early continue in loops.  It tells me: you
> Simon> don't need to think about that case for the rest of the function loop.
> 
> In this particular case the two branches are single lines.

FWIW, one branch is one line, the other 4 lines:
...
       if (addrmap_node_value (n))
         {
           /* Already mapped.  */
           full_range = false;
         }
       else
         addrmap_node_set_value (n, obj);
...

Thanks,
- Tom

> Using a continue here seems more confusing for that reason -- it's just
> more to process here.
> 
> I get it for longer loops, at least situationally.
> 
> Tom