[PATCH] D20260: IR: Introduce local_unnamed_addr attribute.

[David Blaikie via llvm-commits]

On Tue, May 17, 2016 at 9:58 PM, Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> > On May 17, 2016, at 9:48 PM, Justin Bogner <mail at justinbogner.com>
> wrote:
> >
> > Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org> writes:
> >>    On May 17, 2016, at 5:21 PM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
> >>
> >>    On Tue, May 17, 2016 at 5:08 PM Mehdi Amini via llvm-commits <
> >>    llvm-commits at lists.llvm.org> wrote:
> >>
> >>        Seriously I absolutely don't get the "start a new revision" thing
> >>        because the initial email was not on llvm-commit. Really what is
> the
> >>        rational?
> >>
> >>        We are losing the history and the threading for the review, this
> is
> >>        really annoying.
> >>
> >>        So big -1
> >>
> >>    I don't care very much about starting a new revision.
> >>
> >>    Justin and others who don't use phabricator asked for this because
> that is
> >>    what causes an email with a patch file to be sent to llvm-commits,
> and so
> >>    I've been trying to encourage it based on their request.
> >>
> >> There *has been* an email with a patch sent to llvm-commit after the
> first
> >> update to the diff:
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160516/357163.html
> >> Just "having a patch sent to llvm-commit" can be accomplished with an
> manual
> >> email answering to the revision instead of create a new one from
> scratch.
> >
> > Yes, there's a patch, and it explains what's happening as "Minimise the
> > diff a little" and "Add assembler/bitcode test". The description of
> > what's happening is completely missing. The comments so far aren't on
> > the mailing list at all
>
> Re-creating a revision is not gonna help to put this history back on the
> mailing list.
>
>
> > , and somebody who reads this mail first has no
> > idea what's going on. Is the patch something interesting? Should I spend
> > my time following the link and catching up? It also has a subject that
> > starts with "Re:", which makes me and my email filter think I've already
> > read the first patch and decided whether or not to pay attention.
> >
> > Essentially, patches that start without llvm commits don't look like
> > mails that need to be read.
>
> I judge based on the title, and then click on the link.
>
> >
> > The first email in a patch review thread is incredibly important. It
> > summarizes the point of the patch, provides context, and has the patch.
> > Coming into the conversation half way through is very difficult.
>
> We all agree that it is *better* to have llvm-commit as an initial
> subscriber, but unless someone put the manpower in to get it enforced by
> phabricator, we'll have to live with the fact that occasionally it will be
> missing.
>

I'm pretty sure the warning that turns up in Phab suggests creating a new
code review with *-commits on from the start. I'd encourage this to be the
path we encourage/request when we see this (for myself, it's really obvious
when someone creates a review with me as a reviewer but without the mailing
list - such a mail ends up in my inbox, whereas if it had the mailing list
on it it'd be in my mailing list bucket). I think we should do it early and
often. Yes, fixing Phab would be better. But I think restarting the review
is better than continuing it without the original/initial context - earlier
the better, before lots of conversation has already happened off list.


>
> >> That said the patch is always a mere two-clicks away from the email:
> http://
> >> reviews.llvm.org/D20260?download=true
> >> One could even write a plugin for his email client to act on link in
> the email
> >> "http://reviews.llvm.org/D20260" and automatically curl the latest
> patch.
> >
> > How do I know if I need or want to look at it? Is "Minimise the diff a
> > little" an interesting patch?
>
> Seriously?? The email title says "IR: Introduce local_unnamed_addr
> attribute", that pretty clear IMO.
>
> >
> >>    There is still the fact that even the initial phab revision has
> >>    essentially no context. I've skimmed thin three or four times and
> I'm not
> >>    sure yet what the motivation is... I'm sure it has one, I just can't
> find
> >>    it.
> >>
> >> s/Initial phab revision/current phab revision/
> >>
> >> Yes, I'd expect a phabricator revision to have a correct description.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160517/ff6622cc/attachment.html>