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

Mon Dec 9 13:03:03 PST 2019

sivachandra added a comment.

In D71094#1775764 <https://reviews.llvm.org/D71094#1775764>, @abrachet 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.)
>
>
> Sorry about that there ended up being many more changes than I anticipated here when I accepted. I'll be more careful next time.

If it can make you feel better, apart from the minor nits, the "major" changes between the latest version and the version you have accepted is the addition of the const attribute and the new error macros. We have explored the idea of making errno a reference for internal use, but @MaskRay thankfully pointed out to the fact that it leads to a lot more code gen.

> 
> 
> 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.

I also would like to point out that I have addressed each every comment on this review. I did not accept one suggestion, but I have backed it up with some reasoning.

For the sake of progress, I will submit this change soon and move on to other patches.

About, "how reviews should go ... " -- We do not want to be different from any other LLVM code review. I have picked reviewers based on who I feel might have the best feedback for a particular change. I would think that it is the normal practice, which for me personally, has been reinforced with my past experience in the LLVM community.
About, "and what we have in mind for these current revisions." -- If I can come up with something worth while, I will definitely post it to list. If a major design decision has to be made, we should of course have a discussion on the dev list. We have done this for the header generation idea, and going forward I am sure we will have many more such discussions. I do not think it is practical to discuss each and every patch on the list.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094