[PATCH] D20260: IR: Introduce local_unnamed_addr attribute.

Rafael Espíndola via llvm-commits llvm-commits at lists.llvm.org
Wed May 18 11:35:41 PDT 2016 

Or just stop using phab.  It is a tool, it is supposed to make things
better. If I have to constantly remind myself to to this or that I am
better of just not using it.

I am not saying that information should not be added to a thread. I am
saying we should not split it.
On May 18, 2016 1:15 PM, "David Blaikie via llvm-commits" <
llvm-commits at lists.llvm.org> wrote:

>
>
> On Wed, May 18, 2016 at 10:04 AM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>>
>> On May 18, 2016, at 9:53 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Wed, May 18, 2016 at 8:22 AM, Mehdi Amini <mehdi.amini at apple.com>
>> wrote:
>>
>>> Hi David,
>>>
>>>
>>> On May 17, 2016, at 11:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> 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)
>>>
>>>
>>> But why does it have to be in a *new* review? How forwarding the
>>> original email that started the thread not enough to get the initial
>>> context?
>>>
>>>
>>> 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?
>>>
>>>
>>> You didn't get my last bit the way I meant it apparently, let me try
>>> again:
>>>
>>> 1) A revision is created without llvm-commits as a subscriber
>>> 2) I put inline comments in the review and we go through one or multiple
>>> round of reviews.
>>> 3) At some point during the process (before 2 or between two rounds of
>>> review, llvm-commits gets added)
>>> 4) People complains because there didn't get the previous emails and ask
>>> to start a new revision
>>>
>>> Of course if we catch it during 1, we can still act on it (forward the
>>> email to llvm-commits, or nuke the revision and start a new one, same
>>> effect to me), and it seems to me that it is what you're saying, right?
>>> However, the case I am complaining about here, is when we don't catch it
>>> initially and we end up in 4. What do we do now? Restarting a new revision
>>> nukes the review history and all my inline comments are lost, i.e. I'm not
>>> OK to restart on a new revision.
>>>
>>
>> OK, thanks for clarifying. Why would this be unacceptable to you?
>>
>>
>> It is very annoying to me because I rely heavily on the fact that I'll be
>> able to see the new revision with my inline comments to see what was
>> updated compared to what I commented. Or what files are now changed.
>> The ability to see the change between arbitrary revision in Phabricator
>> is also broken when you open a new revision.
>>
>>
>> This is how a non-Phab review would progress, right? (unstructured replies
>>
>>
>> Yes, which is also why I'm not much involved in these (quickly looking in
>> 2016 I was involved in ~200 reviews, and it seems at most 3 or 4 we not on
>> Phab). I.e. we wouldn't have Phab, I would review less.
>>
>>
>>
>> - usually in infix email style, responding to your comments - the same
>> thing could be fairly easily replicated in an email response (by the
>> original author - quoting your off list review emails and interspersing
>> their reply, etc) to the new thread)
>>
>>
>>> So while it is unfortunate that it was not caught earlier, I argue that
>>> the inconvenience for the reviewer should prevail on the inconvenience for
>>> the hypothetical people that may engage in the review: if sending them the
>>> original email is not enough, either they'll have to use a browser, or
>>> they'll still be able to post-commit review.
>>>
>>
>> It's not just a matter of hypothetical reviewers, but also of documenting
>> the review in the public system of record for participants, lurkers, and
>> archaeologists who may need to discover why certain choices were made after
>> the fact. This was always the plan for Phab - that email was still the
>> system of record.
>>
>>
>> Yes, and we agree llvm-commits *should* be there from the start, however
>> in the situation I described above, some round of reviews are not and won't
>> be available on the list, even if you start a new revision.
>>
>>
>> I suppose it comes down to this: I think the best thing would be for a
>> new thread to be created, and the feedback/context ported over from here.
>> So it's got a fresh start and is easy to access for current and future
>> readers (rather than having to come in half way, trawl through the thread,
>> find the original context provided part-way through, etc). There would be
>> some inconvenience for existing reviewers, but much of that could be
>> mitigated by the original patch author porting over the context into that
>> new thread.
>>
>> Not as good (for the review, the community, etc), but probably
>> sufficient, would be to add the original context to this thread to get
>> everyone up to speed on the mailing list.
>>
>>
>> You'll probably have to expect me ranting about it anytime it happens on
>> a review I'm involved with :)
>>
>
> Then perhaps you can help to instill diligence in those who send reviews
> your way to ensure/by ensuring they have been addressed to *-commits before
> beginning the review. My particular mail rule setup makes this mistake very
> obvious - but I imagine there are other ways to make it obvious to you
> depending on how you choose to have your mail rules configured.
>
> - Dave
>
>
>>
>> --
>> Mehdi
>>
>>
>>
>>
>>
>>>
>>>
>>
>>>
>>>
>>>>
>>>>>
>>>>> 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
>>>>>>
>>>>>
>>>
>>
>>
>
> _______________________________________________
> 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/20160518/ef1a3698/attachment.html>

More information about the llvm-commits mailing list