[libc-commits] [PATCH] D155766: [libc] Remove global constructor in `getopt` implementation

[Joseph Huber via Phabricator via libc-commits]

jhuber6 created this revision.
jhuber6 added reviewers: gchatelet, JonChesterfield, sivachandra, lntue, michaelrj, abrachet.
Herald added projects: libc-project, All.
Herald added a subscriber: libc-commits.
jhuber6 requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155766

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


Index: libc/test/src/unistd/getopt_test.cpp
===================================================================
--- libc/test/src/unistd/getopt_test.cpp
+++ libc/test/src/unistd/getopt_test.cpp
@@ -31,7 +31,7 @@
 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
Index: libc/src/unistd/getopt.h
===================================================================
--- libc/src/unistd/getopt.h
+++ 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);
Index: libc/src/unistd/getopt.cpp
===================================================================
--- libc/src/unistd/getopt.cpp
+++ libc/src/unistd/getopt.cpp
@@ -22,7 +22,7 @@
 namespace __llvm_libc {
 
 template <typename T> struct RefWrapper {
-  RefWrapper(T *ptr) : ptr(ptr) {}
+  constexpr RefWrapper(T *ptr) : ptr(ptr) {}
   RefWrapper &operator=(const RefWrapper &) = default;
   operator T &() { return *ptr; }
   T &get() { return *ptr; }
@@ -35,15 +35,20 @@
   RefWrapper<int> optopt;
   RefWrapper<unsigned> optpos;
 
-  int opterr;
+  RefWrapper<int> opterr;
 
   FILE *errstream;
 
   GetoptContext &operator=(const GetoptContext &) = default;
 
   template <typename... Ts> void report_error(const char *fmt, Ts... ts) {
-    if (opterr)
-      __llvm_libc::fprintf(errstream, fmt, ts...);
+    if (opterr) {
+      if (errstream)
+        __llvm_libc::fprintf(errstream, fmt, ts...);
+      else
+        __llvm_libc::fprintf(reinterpret_cast<FILE *>(__llvm_libc::stderr), fmt,
+                             ts...);
+    }
   }
 };
 
@@ -171,25 +176,21 @@
 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D155766.542252.patch
Type: text/x-patch
Size: 2888 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libc-commits/attachments/20230720/4499cf38/attachment.bin>