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

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked an inline comment as done.
sivachandra added inline comments.


================
Comment at: libc/config/linux/api.td:34
+    extern int *__errno_location();
+    #define errno *__errno_location()
+  }];
----------------
MaskRay wrote:
> `()`
> 
> Otherwise `errno[0]` is incorrect.
We don't want to put it in parenthesis because we want to error on uses like `errno[0]`. `errno` is supposed to be an lvalue of int type and allowing `errno[0]` actually amounts to leaking implementation detail.


================
Comment at: libc/spec/stdc.td:176
+      [
+        Macro<"errno">,
+      ],
----------------
abrachet wrote:
> Are adding the errno macros out of the scope of this patch?
The generated header is not coming out pretty (though syntactically correct) with a large number of macros added (the order seems messed up ). I had wanted to first fix that up and then add the remaining macros. But, after reading your comment, it occurs to me that it is better to fix the problem than to hedge it. So, I have added the remaining macros now. Will improve the look of the generated file with a different patch.


================
Comment at: libc/src/errno/errno_test.cpp:13
+
+TEST(ErrnoTest, Smoke) {
+  int test_val = 123;
----------------
abrachet wrote:
> Something like `Basic` seems more conventional than `Smoke` for a test name. What is it supposed to mean? 
Smoke for https://en.wikipedia.org/wiki/Smoke_testing_(software).
I have changed it to `Basic` now anyway.


================
Comment at: libc/src/errno/llvmlibc_errno.h:12
+
+// Internal code should use this macro and not use the errno macro from the
+// public header.
----------------
abrachet wrote:
> Internally we could use a `thread_local int &` instead of relying on a macro, I believe. It seems nicer, what do you think?
That's a good idea so I have done it now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71094