[PATCH] D20260: IR: Introduce local_unnamed_addr attribute.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Tue May 17 23:47:08 PDT 2016


On Tue, May 17, 2016 at 11:40 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On May 17, 2016, at 11:33 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, May 17, 2016 at 11:24 PM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>>
>> On May 17, 2016, at 11:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> 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's the point: *after* some round of reviews were already conducted on
>> Phabricator, the most important context for the review *is* now this review
>> history and this is what is important to preserve, and this is why
>> restarting a new revision in this condition is a non-starter to me.
>>
>
> Except none of that is on the mailing list archive or otherwise preserved
> in LLVM's system of record. It was pretty well discussed when Phab was
> introduced that the mailing list would be the system of record and Phab
> just a tool. If it's not on the list, it's not here.
>
>
> Except it exists, and in the present time it is useful
> context/information, so "it's not here" is definitely not an accurate
> representation of the situation.
>
>
> In the case where a bunch of review without the list - we should
> reintroduce any of that context/discussion.
>
>
> As said before, opening a new review is not gonna make this appear
> magically.
>

I'm not suggesting it appear by magic. I'm suggesting that the person
(Peter) who creates the new review, summarize the existing off-list (&
partly on list as well) discussion in that new review, so everyone's got
the context to engage in this review. (& so it's recorded on the mailing
list for archival purposes, etc)


>
>
> I don't know whether a new thread's especially necessary, but at least an
> authoritative email with all that context being summarized into this thread
> is called for. It'll be harder to follow the thread (people expect the
> first email to have the introductory context, etc) but not impossible.
>
>
> I'd probably favor a new thread, with the old one referenced for
> archaeology - the current state can be summarized, any outstanding comments
> can be replied to in the new thread with any context quoted as needed.
>
>
> If I spent time reviewing a revision, using inline comment on Phabricator,
> and a further round of review is done with a different revision, I don't
> see my inline comments and I can't figure if they were addressed, and I
> can't really see what changed between the two revisions.
>

That just brings us back to where we would be if we didn't have Phab -
that's what we all had to do anyway, read the patch, read someone's
unstructured comments, etc. And other people in the community are seeing a
worse situation - because they're not using/looking at Phab they're missing
all that context .


> This, as a reviewer, is most important than anything else for me, and I
> consider that opening a new revision in this condition is a waste of *my*
> time as a reviewer, and this is for benefit that sounds hand-wavy to me. So
> in the future I'll probably resign as a reviewer on the any new revision
> created in this condition.
>

This last bit seems like it's entirely the point - if you notice a review
created in this condition if the first thing you do is say "this isn't in a
good condition, please create one with the mailing list on it, I'll review
that one" - then we get exactly what we all seem to want, right?

- Dave


>
>
> --
> Mehdi
>
>
>
>
> - Dave
>
>
>
>
>>
>> A revision on Phabricator that has no history can be nuked without
>> consequence of course...
>>
>> --
>> Mehdi
>>
>>
>>
>>
>>>
>>> >> 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/efbb2f0c/attachment.html>


More information about the llvm-commits mailing list