[libc-commits] [PATCH] D71094: [libc] Add implementation of errno and define the other macros of errno.h.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

In D71094#1775691 <https://reviews.llvm.org/D71094#1775691>, @MaskRay wrote:

> @abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)


Sorry about that there ended up being many more changes than I anticipated here when I accepted. I'll be more careful next time.

In D71094#1775694 <https://reviews.llvm.org/D71094#1775694>, @sivachandra wrote:

> As I have said previously, we want to add visibility attributes in a more principled manner. I have also said previously that your suggestion is very valid, and that it was considered even before putting this patch out.
>
> A discussion on libc-dev or llvm-dev will definitely be started when I am prepared with all the details I want to bring up. Or, if someone wants to champion the whole area of interop with other libcs, I would be glad to lean on their help. I do not see why such a discussion should block this patch. FWIW, this patch helps me add more of the library, which I think is of greater importance to keep the development of this library running.


I think @sivachandra and I are on the same page here that things are bound to change later and currently the ground work is being laid so that development can truly start. I think its worth starting a thread on the libc list of how reviews should go and what we have in mind for these current revisions.

In D71094#1775720 <https://reviews.llvm.org/D71094#1775720>, @sivachandra wrote:

> In D71094#1775691 <https://reviews.llvm.org/D71094#1775691>, @MaskRay wrote:
>
> > @abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)
>
>
> I appreciate that you and @abrachet have volunteered to do the reviews here. At the same time, I find the tone of the above comment unfriendly. AFAICT, @abrachet is doing this voluntarily out of his own interest. An unfriendly tone for actions, which I view as "friendly community citizenship", definitely is uncalled for.


FWIW I don't find @MaskRay's comments to be unfriendly, and I value his opinion a lot, he has helped me on my patches quite a bit in the past. He simply pointed out something I could do better, and I'm glad he did.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71094/new/

https://reviews.llvm.org/D71094