From: "Metzger, Markus T" <markus.t.metzger@intel.com>
To: Simon Marchi <simon.marchi@polymtl.ca>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 3/3] btrace: Remove ui_out cleanups
Date: Tue, 06 Mar 2018 07:30:00 -0000 [thread overview]
Message-ID: <A78C989F6D9628469189715575E55B236964FF25@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <cb965107ba995343612edc8afd7b92b7@polymtl.ca>
Hello Simon,
> >> static void
> >> btrace_print_lines (struct btrace_line_range lines, struct ui_out
> >> *uiout,
> >> - struct cleanup **ui_item_chain, int flags)
> >> + gdb::optional<ui_out_emit_tuple> *src_and_asm_tuple,
> >> + gdb::optional<ui_out_emit_list> *asm_list,
> >
> > Reference instead of pointer?
>
> I once pointed this out on one of Tom's patches, and he said that in the
> caller code, it's more obvious that object is meant to be modified if
> you do:
>
> function_that_modifies_object (&object);
>
> instead of
>
> function_that_modifies_object (object);
>
> And I kind of agree with that it is, which is why I've been using
> pointers when references would have worked (ok, here the function name
> makes it obvious but it's not always the case). In the implementation
> of the function, since you use object->field instead of object.field, it
> also hints that you're not modifying a local object. But I also agree
> that it's really not C++-y to do it this way, so I'll happily change it.
I prefer consistency. I we agreed to use pointers instead of references
in other parts of GDB, let's do so everywhere.
> Yes, I have ran the gdb.btrace/*.exp tests locally on two different
> machines and saw no regressions. However, the processors may be a bit
> old (Q6600 from 2007 and i5-4310U from 2014), so it's possible that not
> all required features are available, and therefore some tests may be
> skipped. So if you want to be sure, here's a branch for you to test:
You would get an "untested" if btrace tests are skipped. As long as
you're not getting all "untested", you should be fine. There is only
one test, tsx.exp, that requires recent hardware and compiler.
It would use the method that is available on your target preferring
PT over BTS. But this change is not related to trace decode so it
shouldn't matter.
I ran the tests on recent hardware using PT and everything passes.
Thanks,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
next prev parent reply other threads:[~2018-03-06 7:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-04 20:56 [PATCH 1/3] btrace: Remove btrace disable cleanup Simon Marchi
2018-03-04 20:56 ` [PATCH 2/3] btrace: Remove VEC cleanups Simon Marchi
2018-03-05 12:39 ` Metzger, Markus T
2018-03-04 20:56 ` [PATCH 3/3] btrace: Remove ui_out cleanups Simon Marchi
2018-03-05 12:39 ` Metzger, Markus T
2018-03-05 22:16 ` Simon Marchi
2018-03-06 7:30 ` Metzger, Markus T [this message]
2018-03-06 14:40 ` Simon Marchi
2018-03-05 12:39 ` [PATCH 1/3] btrace: Remove btrace disable cleanup Metzger, Markus T
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=A78C989F6D9628469189715575E55B236964FF25@IRSMSX104.ger.corp.intel.com \
--to=markus.t.metzger@intel.com \
--cc=gdb-patches@sourceware.org \
--cc=simon.marchi@polymtl.ca \
/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