From: Lancelot SIX <Lancelot.Six@amd.com>
To: Pedro Alves <pedro@palves.net>,
Andrew Burgess <aburgess@redhat.com>,
Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>
Cc: lsix@lancelotsix.com
Subject: Re: Formatting/indentation of lambdas (Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj)
Date: Tue, 5 Jul 2022 14:33:17 +0100 [thread overview]
Message-ID: <2dd3b458-634f-6c53-0264-c5f60329762c@amd.com> (raw)
In-Reply-To: <39b8b2c1-0aee-e830-92bb-e37256b183f6@palves.net>
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).
next prev parent reply other threads:[~2022-07-05 13:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-17 10:10 [PATCH 0/3] Fix some use-after-free errors in varobj code Lancelot SIX
2022-06-17 10:10 ` [PATCH 1/3] MI: mi_runto -pending Lancelot SIX
2022-06-17 10:10 ` [PATCH 2/3] gdb/varobj: Fix use after free in varobj Lancelot SIX
2022-06-17 16:09 ` Andrew Burgess
2022-06-17 16:38 ` Lancelot SIX
2022-06-20 15:52 ` Lancelot SIX
2022-06-30 18:43 ` Formatting/indentation of lambdas (Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj) Pedro Alves
2022-07-05 13:33 ` Lancelot SIX [this message]
2022-06-17 10:10 ` [PATCH 3/3] gdb/varobj: Fix varobj_invalidate_iter Lancelot SIX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2dd3b458-634f-6c53-0264-c5f60329762c@amd.com \
--to=lancelot.six@amd.com \
--cc=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=lsix@lancelotsix.com \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).