public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: Petter Tomner <tomner@kth.se>
To: David Malcolm <dmalcolm@redhat.com>,
	Antoni Boucher <bouanto@zoho.com>,
	"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	"jit@gcc.gnu.org" <jit@gcc.gnu.org>
Subject: SV: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
Date: Tue, 14 Dec 2021 00:30:08 +0000	[thread overview]
Message-ID: <2ff48f6a7b00476ab0b4d461ea079636@kth.se> (raw)
In-Reply-To: <f2c91e82d59a9ef61d5ba97548511284a8d2a4ca.camel@redhat.com>

Hi!

Thanks for the review. Having proper grammar in user facing text is important so 
I in no way feel it is overzealous. I'll push the patch once I fixed the points mentioned 
in the review.

Regards, Petter

Från: David Malcolm <dmalcolm@redhat.com>
Skickat: den 13 december 2021 20:22
Till: Antoni Boucher; Petter Tomner; gcc-patches@gcc.gnu.org; jit@gcc.gnu.org
Ämne: Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
    
On Sun, 2021-12-12 at 20:39 -0500, Antoni Boucher wrote:
> Yes, this patch works for rustc_codegen_gcc perfectly.
> It even fixes one issue that was in my patch, so that's nice!

Excellent - thanks Antoni.

> 
> Le samedi 11 décembre 2021 à 15:35 +0000, Petter Tomner a écrit :
> > Hi!
> > 
> > > s/an union/a union/
> > > s/a rvalue/an rvalue/
> > 
> > Heh no way ... and I though I knew English grammar :)

I apologize for the non-regularity of English :)  [and it's somewhat
unfair for me to nit-pick, given that it's my first language - sorry;
we may as well get this stuff correct]

> > 
> > Had to look that up to see what the deal was but it makes sense. 
> > 
> > yunion, arevalue.

Exactly.  English spelling is a mess.

> > 
> > > s/wrong-field-name/wrong-field-obj/
> > > 
> > > to match the struct example (given that the issue being tested
> > > for
> > > is
> > > that it's the wrong object, rather than the wrong name).
> > 
> > Initially, before submitting to the list, I made the code such that
> > the field 
> > objects did not have to be the ones that were used when creating
> > the 
> > struct or union, and forgot changing the test names. 
> > 
> > I figured it required too much string compares for the field names
> > and 
> > pointer compares for the field object were more appropriate. To
> > create
> > dummy field objects were also kinda heavy.
> > 
> > I'll address the points.


Petter, you can go ahead and commit this, once you address the various
points from my earlier review, and from Antoni's observation that
"flags" should be "m_flags" when it's a member.  Antoni has committed
at least one ABI version bump since you posted this, so you'll need to
rebase your work on top of his and adjust accordingly.  Please re-test
the patch before pushing it.

Thanks for all your great work on this.
Dave


    

  reply	other threads:[~2021-12-14  0:30 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-06 10:47 Petter Tomner
2021-12-09 19:39 ` David Malcolm
2021-12-11 15:35   ` SV: " Petter Tomner
2021-12-13  1:39     ` Antoni Boucher
2021-12-13 19:22       ` David Malcolm
2021-12-14  0:30         ` Petter Tomner [this message]
2021-12-13  1:43     ` Antoni Boucher
2021-12-13  1:48     ` Antoni Boucher

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=2ff48f6a7b00476ab0b4d461ea079636@kth.se \
    --to=tomner@kth.se \
    --cc=bouanto@zoho.com \
    --cc=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jit@gcc.gnu.org \
    /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).