fix lint error in wgettext()

Cppcheck was complaining about the `return val.c_str()` at the end of the
`wgettext()` function. That would normally a bug since the lifetime of
`val` ends when the function returns. In this particular case that's not
true because the string is interned in a cache. Nonetheless, rather than
suppress the lint warning I decided to modify the API to be more idiomatic.

In the process of fixing the aforementioned lint warning I fixed several other
lint errors in that module.

This required making our copy of `wgetopt()` compatible with the rest of
the fish code. Specifically, by removing its local definitions of the
"_" macro so it uses the same macro used everywhere else in the fish
code. The sooner we kill the use of wide chars the better.

configure.ac

@@ -237,18 +237,6 @@ LDFLAGS="$prev_LDFLAGS"
 
 AC_CHECK_FILES([/proc/self/stat])
 
-
-#
-# This is ued to tell the wgetopt library to translate strings. This
-# way wgetopt can be dropped into any project without requiring i18n.
-#
-
-AC_DEFINE(
-  [HAVE_TRANSLATE_H],
-  [1],
-  [Define to 1 if the wgettext function should be used for translating strings.]
-)
-
 # Disable curses macros that conflict with the STL
 AC_DEFINE([NCURSES_NOMACROS], [1], [Define to 1 to disable ncurses macros that conflict with the STL])
 AC_DEFINE([NOMACROS], [1], [Define to 1 to disable curses macros that conflict with the STL])

src/common.h

@@ -226,8 +226,8 @@ void write_ignore(int fd, const void *buff, size_t count);
         return retval;                                                     \
     }
 
-/// Shorthand for wgettext call.
-#define _(wstr) wgettext(wstr)
+/// Shorthand for wgettext call in situations where a C-style string is needed (e.g., fwprintf()).
+#define _(wstr) wgettext(wstr).c_str()
 
 /// Noop, used to tell xgettext that a string should be translated, even though it is not directly
 /// sent to wgettext.

src/complete.cpp

@@ -56,7 +56,9 @@
 /// since it can occur, and should not be translated. (Gettext returns the version information as
 /// the response).
 #ifdef USE_GETTEXT
-static const wchar_t *C_(const wcstring &s) { return s.empty() ? L"" : wgettext(s.c_str()); }
+static const wchar_t *C_(const wcstring &s) {
+    return s.empty() ? L"" : wgettext(s.c_str()).c_str();
+}
 #else
 static const wcstring &C_(const wcstring &s) { return s; }
 #endif
@@ -193,12 +195,13 @@ completion_t::~completion_t() {}
 // Clear the COMPLETE_AUTO_SPACE flag, and set COMPLETE_NO_SPACE appropriately depending on the
 // suffix of the string.
 static complete_flags_t resolve_auto_space(const wcstring &comp, complete_flags_t flags) {
+    complete_flags_t new_flags = flags;
     if (flags & COMPLETE_AUTO_SPACE) {
-        flags = flags & ~COMPLETE_AUTO_SPACE;
+        new_flags &= ~COMPLETE_AUTO_SPACE;
         size_t len = comp.size();
-        if (len > 0 && (wcschr(L"/=@:", comp.at(len - 1)) != 0)) flags |= COMPLETE_NO_SPACE;
+        if (len > 0 && (wcschr(L"/=@:", comp.at(len - 1)) != 0)) new_flags |= COMPLETE_NO_SPACE;
     }
-    return flags;
+    return new_flags;
 }
 
 // completion_t functions. Note that the constructor resolves flags!

src/wgetopt.cpp

@@ -36,7 +36,7 @@
 // You should have received a copy of the GNU Library General Public License along with the GNU C
 // Library; see the file COPYING.LIB.  If not, write to the Free Software Foundation, Inc., 675 Mass
 // Ave, Cambridge, MA 02139, USA.
-#include "config.h"
+#include "config.h"  // IWYU pragma: keep
 
 #include <stdio.h>
 #include <wchar.h>
@@ -62,21 +62,6 @@
 #include "wgetopt.h"
 #include "wutil.h"  // IWYU pragma: keep
 
-// Use translation functions if available.
-#ifdef _
-#undef _
-#endif
-
-#ifdef HAVE_TRANSLATE_H
-#ifdef USE_GETTEXT
-#define _(string) wgettext(string)
-#else
-#define _(string) (string)
-#endif
-#else
-#define _(wstr) wstr
-#endif
-
 #ifdef __GNU_LIBRARY__
 // We want to avoid inclusion of string.h with non-GNU libraries because there are many ways it can
 // cause trouble. On some systems, it contains special magic macros that don't work in GCC.
@@ -306,9 +291,8 @@ int wgetopter_t::_wgetopt_internal(int argc, wchar_t **argv, const wchar_t *opts
         int indfound = 0;  // set to zero by Anton
         int option_index;
 
-        for (nameend = nextchar; *nameend && *nameend != '='; nameend++) {
-            // Do nothing.
-        }
+        for (nameend = nextchar; *nameend && *nameend != '='; nameend++)
+            ;  //!OCLINT(empty body)
 
         // Test all long options for either exact match or abbreviated matches.
         for (p = longopts, option_index = 0; p->name; p++, option_index++)

src/wutil.cpp

@@ -166,7 +166,6 @@ FILE *wfopen(const wcstring &path, const char *mode) {
         default: {
             errno = EINVAL;
             return NULL;
-            break;
         }
     }
     // Skip binary.
@@ -196,18 +195,22 @@ bool set_cloexec(int fd) {
 static int wopen_internal(const wcstring &pathname, int flags, mode_t mode, bool cloexec) {
     ASSERT_IS_NOT_FORKED_CHILD();
     cstring tmp = wcs2string(pathname);
-// Prefer to use O_CLOEXEC. It has to both be defined and nonzero.
+    int fd;
+
 #ifdef O_CLOEXEC
-    if (cloexec && (O_CLOEXEC != 0)) {
-        flags |= O_CLOEXEC;
-        cloexec = false;
+    // Prefer to use O_CLOEXEC. It has to both be defined and nonzero.
+    if (cloexec) {
+        fd = open(tmp.c_str(), flags | O_CLOEXEC, mode);
+    } else {
+        fd = open(tmp.c_str(), flags, mode);
     }
-#endif
-    int fd = ::open(tmp.c_str(), flags, mode);
-    if (cloexec && fd >= 0 && !set_cloexec(fd)) {
+#else
+    fd = open(tmp.c_str(), flags, mode);
+    if (fd >= 0 && !set_cloexec(fd)) {
         close(fd);
         fd = -1;
     }
+#endif
     return fd;
 }
 
@@ -251,7 +254,8 @@ void wperror(const wchar_t *s) {
 int make_fd_nonblocking(int fd) {
     int flags = fcntl(fd, F_GETFL, 0);
     int err = 0;
-    if (!(flags & O_NONBLOCK)) {
+    bool nonblocking = flags & O_NONBLOCK;
+    if (!nonblocking) {
         err = fcntl(fd, F_SETFL, flags | O_NONBLOCK);
     }
     return err == -1 ? errno : 0;
@@ -260,7 +264,8 @@ int make_fd_nonblocking(int fd) {
 int make_fd_blocking(int fd) {
     int flags = fcntl(fd, F_GETFL, 0);
     int err = 0;
-    if (flags & O_NONBLOCK) {
+    bool nonblocking = flags & O_NONBLOCK;
+    if (nonblocking) {
         err = fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
     }
     return err == -1 ? errno : 0;
@@ -406,17 +411,13 @@ static void wgettext_init_if_necessary() {
     pthread_once(&once, wgettext_really_init);
 }
 
-const wchar_t *wgettext(const wchar_t *in) {
-    if (!in) return in;
-
+const wcstring &wgettext(const wchar_t *in) {
     // Preserve errno across this since this is often used in printing error messages.
     int err = errno;
+    wcstring key = in;
 
     wgettext_init_if_necessary();
-
-    wcstring key = in;
     scoped_lock lock(wgettext_lock);  //!OCLINT(has side effects)
-
     wcstring &val = wgettext_map[key];
     if (val.empty()) {
         cstring mbs_in = wcs2string(key);
@@ -427,7 +428,7 @@ const wchar_t *wgettext(const wchar_t *in) {
 
     // The returned string is stored in the map.
     // TODO: If we want to shrink the map, this would be a problem.
-    return val.c_str();
+    return val;
 }
 
 int wmkdir(const wcstring &name, int mode) {

src/wutil.h

@@ -84,7 +84,7 @@ std::wstring wbasename(const std::wstring &path);
 /// gettext function, wgettext takes care of setting the correct domain, etc. using the textdomain
 /// and bindtextdomain functions. This should probably be moved out of wgettext, so that wgettext
 /// will be nothing more than a wrapper around gettext, like all other functions in this file.
-const wchar_t *wgettext(const wchar_t *in);
+const wcstring &wgettext(const wchar_t *in);
 
 /// Wide character version of mkdir.
 int wmkdir(const wcstring &dir, int mode);