[libc-commits] [libc] ed7ade9 - [libc] Remove global constructor in `getopt` implementation

[Joseph Huber via libc-commits]

Author: Joseph Huber
Date: 2023-07-20T08:11:19-05:00
New Revision: ed7ade9cfa56acae3c67bdf837dec05555769e3c

URL: https://github.com/llvm/llvm-project/commit/ed7ade9cfa56acae3c67bdf837dec05555769e3c
DIFF: https://github.com/llvm/llvm-project/commit/ed7ade9cfa56acae3c67bdf837dec05555769e3c.diff

LOG: [libc] Remove global constructor in `getopt` implementation

This file required a global constructor due to copying the file stream
and have a non-constexpr constructor for the wrapper type. Also, I
changes the `opterr` to be a pointer, because it seemed like it wasn't
being set correctly as an externally visibile variable if we just
captured it by value.

Reviewed By: abrachet

Differential Revision: https://reviews.llvm.org/D155766

Added: 
    

Modified: 
    libc/src/unistd/getopt.cpp
    libc/src/unistd/getopt.h
    libc/test/src/unistd/getopt_test.cpp

Removed: 
    


################################################################################
diff  --git a/libc/src/unistd/getopt.cpp b/libc/src/unistd/getopt.cpp
index 4c6aaf629dbe51..70ca454315d3ed 100644
--- a/libc/src/unistd/getopt.cpp
+++ b/libc/src/unistd/getopt.cpp
@@ -22,7 +22,6 @@
 namespace __llvm_libc {
 
 template <typename T> struct RefWrapper {
-  RefWrapper(T *ptr) : ptr(ptr) {}
   RefWrapper &operator=(const RefWrapper &) = default;
   operator T &() { return *ptr; }
   T &get() { return *ptr; }
@@ -35,7 +34,7 @@ struct GetoptContext {
   RefWrapper<int> optopt;
   RefWrapper<unsigned> optpos;
 
-  int opterr;
+  RefWrapper<int> opterr;
 
   FILE *errstream;
 
@@ -43,7 +42,9 @@ struct GetoptContext {
 
   template <typename... Ts> void report_error(const char *fmt, Ts... ts) {
     if (opterr)
-      __llvm_libc::fprintf(errstream, fmt, ts...);
+      __llvm_libc::fprintf(
+          errstream ? errstream : reinterpret_cast<FILE *>(__llvm_libc::stderr),
+          fmt, ts...);
   }
 };
 
@@ -171,25 +172,21 @@ int getopt_r(int argc, char *const argv[], const char *optstring,
 namespace impl {
 
 extern "C" {
-
 char *optarg = nullptr;
 int optind = 1;
 int optopt = 0;
 int opterr = 0;
-
 }
 
 static unsigned optpos;
 
-static GetoptContext ctx{
-    &impl::optarg, &impl::optind,
-    &impl::optopt, &optpos,
-    impl::opterr,  reinterpret_cast<FILE *>(__llvm_libc::stderr)};
+static GetoptContext ctx{&impl::optarg, &impl::optind, &impl::optopt,
+                         &optpos,       &impl::opterr, /*errstream=*/nullptr};
 
 #ifndef LIBC_COPT_PUBLIC_PACKAGING
 // This is used exclusively in tests.
 void set_getopt_state(char **optarg, int *optind, int *optopt, unsigned *optpos,
-                      int opterr, FILE *errstream) {
+                      int *opterr, FILE *errstream) {
   ctx = {optarg, optind, optopt, optpos, opterr, errstream};
 }
 #endif

diff  --git a/libc/src/unistd/getopt.h b/libc/src/unistd/getopt.h
index bf5f8d040c9441..0bc7ed6a1b1934 100644
--- a/libc/src/unistd/getopt.h
+++ b/libc/src/unistd/getopt.h
@@ -15,7 +15,7 @@
 namespace __llvm_libc {
 
 namespace impl {
-void set_getopt_state(char **, int *, int *, unsigned *, int, FILE *);
+void set_getopt_state(char **, int *, int *, unsigned *, int *, FILE *);
 }
 
 int getopt(int argc, char *const argv[], const char *optstring);

diff  --git a/libc/test/src/unistd/getopt_test.cpp b/libc/test/src/unistd/getopt_test.cpp
index b90ea6499e0d0d..546138ab78dd8b 100644
--- a/libc/test/src/unistd/getopt_test.cpp
+++ b/libc/test/src/unistd/getopt_test.cpp
@@ -31,7 +31,7 @@ unsigned optpos;
 void set_state(FILE *errstream) {
   __llvm_libc::impl::set_getopt_state(
       &test_globals::optarg, &test_globals::optind, &test_globals::optopt,
-      &test_globals::optpos, test_globals::opterr, errstream);
+      &test_globals::optpos, &test_globals::opterr, errstream);
 }
 
 // TODO: <stdio> could be either llvm-libc's or the system libc's. The former