How to get a review for a patch?

Hi all,

I submitted a small documentation patch [1] three weeks ago. This is my first
patch submitted against LLVM, so I was extra careful to follow the instructions
(turns out this is quite a bit more complicated than the pull-request-based
workflow). So far, I got no reaction at all to my patch. Does that mean I did
something wrong, like picking the wrong reviewer? Should I pick another one?
Obviously I don't know the code organization nor any of the people involved, so
I don't feel I can meaningfully judge who might be able to review this patch.

[1]: ⚙ D57600 update docs of memcpy/memmove/memset re: alignment and len=0

Kind regards,
Ralf

Hi Ralf,

You added the old account for Eli (eli.friedman); I went ahead and switched it to the newer account (efriedma). You can tell it’s an old account because if you go to https://reviews.llvm.org/p/eli.friedman/ (which can be accessed by e.g. clicking the eli.friedman in your reviewers list), the last activity is from 2016, whereas https://reviews.llvm.org/p/efriedma/ has recent activity. Hopefully that gets you some activity. It’s also customary to add llvm-commits as a subscriber instead of a reviewer, but that shouldn’t make too much of a difference.

The standard procedure is also to ping reviews weekly, which helps get review activity in most cases.

Hi Shoaib,

You added the old account for Eli (eli.friedman); I went ahead and switched it
to the newer account (efriedma). You can tell it's an old account because if you
go to https://reviews.llvm.org/p/eli.friedman/ (which can be accessed by e.g.
clicking the eli.friedman in your reviewers list), the last activity is from
2016, whereas https://reviews.llvm.org/p/efriedma/ has recent activity.
Hopefully that gets you some activity.

Thanks a lot! That seems like an easy trap to run into. Is there a way to not
suggest the old accounts (in the drop-down menu) when adding reviewers?

It's also customary to add llvm-commits
as a subscriber instead of a reviewer, but that shouldn't make too much of a
difference.

Thanks, I'll try to remember this for next time.
I did this based on the following text in [the
docs](https://llvm.org/docs/Phabricator.html#phabricator-request-review-web):

Add reviewers (see below for advice). (If you set the Repository field correctly, llvm-commits or cfe-commits will be subscribed automatically; otherwise, you will have to manually subscribe them.)

I was not aware of there being separate notions of "reviewers" and
"subscribers", so with this being in the "Add reviewers" not I thought "to
subscribe" meant "to add as a reviewer". Actually from what I recall,
llvm-commits had been added automatically (but I might misremember).
(I hope this kind of feedback helps to improve the documentation.)

Kind regards,
Ralf

Comments inline.

From: llvm-dev <llvm-dev-bounces@lists.llvm.org> On Behalf Of Ralf Jung via
llvm-dev
Sent: Tuesday, February 26, 2019 3:21 AM
To: Shoaib Meenai <smeenai@fb.com>; llvm-dev <llvm-dev@lists.llvm.org>
Subject: [EXT] Re: [llvm-dev] How to get a review for a patch?

Hi Shoaib,

> You added the old account for Eli (eli.friedman); I went ahead and switched it
> to the newer account (efriedma). You can tell it's an old account because if
you
> go to ♟ eli.friedman (which can be accessed by e.g.
> clicking the eli.friedman in your reviewers list), the last activity is from
> 2016, whereas https://reviews.llvm.org/p/efriedma/ has recent activity.
> Hopefully that gets you some activity.

Thanks a lot! That seems like an easy trap to run into. Is there a way to not
suggest the old accounts (in the drop-down menu) when adding reviewers?

I don't think there's any way in general to avoid old accounts. IIRC there might be a way I can mark that specific account as dead; I'll look into it. That said, if llvm-commits is CC'ed, I have my filters set so I'll see it anyway. It's my fault I didn't reply; I'm pretty sure I got the notification, it just got buried in other email.

If you don't get a review within a week, please reply to the patch (adding a comment "ping" is enough). And if you don't get a reply in the week after that, escalating to llvmdev is the right thing to do.

> It's also customary to add llvm-commits
> as a subscriber instead of a reviewer, but that shouldn't make too much of a
> difference.

Thanks, I'll try to remember this for next time.
I did this based on the following text in [the
docs](https://llvm.org/docs/Phabricator.html#phabricator-request-review-
web):

> Add reviewers (see below for advice). (If you set the Repository field correctly,
llvm-commits or cfe-commits will be subscribed automatically; otherwise, you
will have to manually subscribe them.)

I was not aware of there being separate notions of "reviewers" and
"subscribers", so with this being in the "Add reviewers" not I thought "to
subscribe" meant "to add as a reviewer". Actually from what I recall,
llvm-commits had been added automatically (but I might misremember).
(I hope this kind of feedback helps to improve the documentation.)

If you accidentally add llvm-commits as a reviewer instead of a subscriber, it doesn't really matter; Phabricator still sends the same email. It's only really an issue if you forget to add it at all.

-Eli

Hi,

Hi,

I was not aware of there being separate notions of "reviewers" and
"subscribers", so with this being in the "Add reviewers" not I thought "to
subscribe" meant "to add as a reviewer". Actually from what I recall,
llvm-commits had been added automatically (but I might misremember).
(I hope this kind of feedback helps to improve the documentation.)

There’s Contributing to LLVM — LLVM 16.0.0git documentation which would ideally contain all
relevant info, but it might be hard to discover. Did you have a look at that page?

I had found that, yes, bit it took me a while. It is from there that I reached
<https://llvm.org/docs/Phabricator.html#phabricator-request-review-web&gt;\.
I started at <https://llvm.org/&gt; and scanned for "contribute", and that went
nowhere at first. Even when I found <https://llvm.org/docs/&gt; it wasn't at all
obvious that this was the right direction, the contribution documentation is
*way* down at the bottom.

Given that <https://github.com/llvm/llvm-project/&gt; is on GH, it might be worth
adding a CONTRIBUTING.md or at least adding some pointers to the README --
that's actually the place I started looking for information.

Kind regards,
Ralf

Hi