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

Mon Dec 9 13:12:07 PST 2019

MaskRay added a comment.

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.

Apologize if you feel that my comments were not friendly. I did not intend that.

> My reason to accept this now is that secondary issues like visibility are almost certainly going to need revisiting later, when we have more content to work with. For instance, do we do it by adding attributes everywhere in sight, or do we build some kind of script or other automation? Given that this is being developed incrementally from zero, we are just making extra unnecessary work by insisting that the defacto prototype solve all the problems that might need solving in the future. So let's get the core of the system figured out now, and not worry about secondary issues until more of the system is in place.

My feeling is that making the attribute change part of the patch is preferable. It makes the intention clear what symbols are public and what symbols and internal implementation details. Adding errno in an incremental step looks great to me. But making attribute changes separate is not. From the comments, it seems that `__errno_location` may be overlayed on top of an existing libc? I am a bit more concerned about the approach that will be used to do the overlay work. The design choice made here needs to account for standalone libc mode + overlay mode. I think we need to make the two modes clear.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094