Formatting/indentation of lambdas (Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj)

[Lancelot SIX]

Hi Pedro,

Thanks a lot for this.  I find conventions you describe appropriate, and 
the guidelines will be useful as I always wonder how to properly indent 
lambdas in GDB codebase.

I have updated the wiki to mention the rules you describe above: 
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Indentation_of_lambdas_as_parameters

If someone objects to the rules, we can always revert / update the wiki 
accordingly.

Best,
Lancelot.

On 30/06/2022 19:43, Pedro Alves wrote:
> [CAUTION: External Email]
> 
> On 2022-06-17 17:09, Andrew Burgess via Gdb-patches wrote:
> 
>>>   /* Update the internal variables and value history when OBJFILE is
>>>      discarded; we must copy the types out of the objfile.  New global types
>>>      will be created for every convenience variable which currently points to
>>> @@ -2617,6 +2633,11 @@ preserve_values (struct objfile *objfile)
>>>     for (var = internalvars; var; var = var->next)
>>>       preserve_one_internalvar (var, objfile, copied_types.get ());
>>>
>>> +  /* For the remaining varobj, check that none has type owned by OBJFILE.  */
>>> +  all_root_varobjs ([&copied_types, objfile](struct varobj *varobj)
>>> +                { preserve_one_varobj (varobj, objfile,
>>> +                                       copied_types.get ()); });
>>> +
>>
>> I think the formatting here is a little off.  Looking through other
>> examples in GDB I think the most common layout would be:
>>
>>    all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
>>                      {
>>                      preserve_one_varobj (varobj, objfile,
>>                                           copied_types.get ());
>>                    });
> 
> For for-each-like functions that take a lambda, I like to indent the body
> of lambda as-if you really had a for loop.  Like:
> 
>    all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
>      {
>        preserve_one_varobj (varobj, objfile, copied_types.get ());
>      });
> 
> which looks similar to what you'd have if you had a real for, like:
> 
>    for (varobj *varobj: root_varobjs ())
>      {
>        preserve_one_varobj (varobj, objfile, copied_types.get ());
>      }
> 
> 
> I'll give you at least a couple existing examples.  E.g., in gdb/linux-nat.c:
> 
>    /* No use iterating unless we're resuming other threads.  */
>    if (scope_ptid != lp->ptid)
>      iterate_over_lwps (scope_ptid, [=] (struct lwp_info *info)
>        {
>          return linux_nat_resume_callback (info, lp);
>        });
> 
> and in gdbserver, any for_each_thread call, like:
> 
>    for_each_thread ([&] (thread_info *thread)
>      {
>        handle_qxfer_threads_worker (thread, buffer);
>      });
> 
> I've noticed that LLVM also uses this style.  It is nicely described here:
> 
>   https://llvm.org/docs/CodingStandards.html#format-lambdas-like-blocks-of-code
> 
> For the case where we have multiple lambdas in a single function call, or when the
> lambda isn't the last argument, I agree with LLVM, adjusted for GNU style.  As in, indent
> like other parameters. An example for this is expand_symtabs_matching, which takes
> several gdb::function_view arguments.  Here's an example call:
> 
>    /* Look through the partial symtabs for all symbols which begin by
>       matching SYM_TEXT.  Expand all CUs that you find to the list.  */
>    expand_symtabs_matching (NULL,
>                             lookup_name,
>                             NULL,
>                             [&] (compunit_symtab *symtab) /* expansion notify */
>                               {
>                                 add_symtab_completions (symtab,
>                                                         tracker, mode, lookup_name,
>                                                         sym_text, word, code);
>                                 return true;
>                               },
>                             SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
>                             ALL_DOMAIN);
> 
> I suggest we follow these conventions, and document it in the internals manual,
> so we have a url we can point to the next time this comes up (it's not the first time).