replace writestr() with fwprintf() in reader.cpp

There are several places that use writestr() which should instead be
using fwprintf() or equivalent. Also, clarify the documentation for why
writestr() and writechr() exist so they aren't used inappropriately
again.

Fixes #3657

src/common.h

@@ -199,12 +199,12 @@ void write_ignore(int fd, const void *buff, size_t count);
 /// the tty.
 extern bool has_working_tty_timestamps;
 
-/// This macro is used to check that an input argument is not null. It is a bit lika a non-fatal
+/// This macro is used to check that an argument is true. It is a bit like a non-fatal form of
-/// form of assert. Instead of exit-ing on failure, the current function is ended at once. The
+/// assert. Instead of exiting on failure, the current function is ended at once. The second
-/// second parameter is the return value of the current function on failure.
+/// parameter is the return value of the current function on failure.
 #define CHECK(arg, retval)                                                               \
     if (!(arg)) {                                                                        \
-        debug(0, "function %s called with null value for argument %s. ", __func__, #arg); \
+        debug(0, "function %s called with false value for argument %s", __func__, #arg); \
         bugreport();                                                                     \
         show_stackframe(L'E');                                                           \
         return retval;                                                                   \

src/output.cpp

@@ -301,7 +301,11 @@ int writeb(tputs_arg_t b) {
     return 0;
 }
 
-/// Write a wide character using the output method specified using output_set_writer().
+/// Write a wide character using the output method specified using output_set_writer(). This should
+/// only be used when writing characters from user supplied strings. This is needed due to our use
+/// of the ENCODE_DIRECT_BASE mechanism to allow the user to specify arbitrary byte values to be
+/// output. Such as in a `printf` invocation that includes literal byte values such as `\x1B`.
+/// This should not be used for writing non-user supplied characters.
 int writech(wint_t ch) {
     char buff[MB_LEN_MAX + 1];
     size_t len;
@@ -328,15 +332,16 @@ int writech(wint_t ch) {
     return 0;
 }
 
-/// Write a wide character string to FD 1.
+/// Write a wide character string to stdout. This should not be used to output things like warning
+/// messages; just use debug() or fwprintf() for that. It should only be used to output user
+/// supplied strings that might contain literal bytes; e.g., "\342\224\214" from issue #1894. This
+/// is needed because those strings may contain chars specially encoded using ENCODE_DIRECT_BASE.
 void writestr(const wchar_t *str) {
     CHECK(str, );
 
-    if (MB_CUR_MAX == 1)  // single-byte locale (C/POSIX/ISO-8859)
+    if (MB_CUR_MAX == 1) {
-    {
+        // Single-byte locale (C/POSIX/ISO-8859).
-        while (*str) {
+        while (*str) writech(*str++);
-            writech(*str++);
-        }
         return;
     }
 
@@ -349,11 +354,11 @@ void writestr(const wchar_t *str) {
     // Convert the string.
     len++;
     char *buffer, static_buffer[256];
-    if (len <= sizeof static_buffer)
+    if (len <= sizeof static_buffer) {
         buffer = static_buffer;
-    else
+    } else {
         buffer = new char[len];
-
+    }
     wcstombs(buffer, str, len);
 
     // Write the string.

src/reader.cpp

@@ -698,17 +698,17 @@ void reader_write_title(const wcstring &cmd, bool reset_cursor_position) {
     proc_push_interactive(0);
     if (exec_subshell(fish_title_command, lst, false /* ignore exit status */) != -1 &&
         !lst.empty()) {
-        writestr(L"\x1b]0;");
+        fputs("\e]0;", stdout);
         for (size_t i = 0; i < lst.size(); i++) {
-            writestr(lst.at(i).c_str());
+            fputws(lst.at(i).c_str(), stdout);
         }
-        writestr(L"\7");
+        fputc('\a', stdout);
     }
     proc_pop_interactive();
     set_color(rgb_color_t::reset(), rgb_color_t::reset());
     if (reset_cursor_position && !lst.empty()) {
         // Put the cursor back at the beginning of the line (issue #2453).
-        writestr(L"\r");
+        fputc('\r', stdout);
     }
 }
 
@@ -1300,7 +1300,7 @@ static void reader_flash() {
     }
 
     reader_repaint();
-    writestr(L"\a");
+    fputwc(L'\a', stdout);
 
     pollint.tv_sec = 0;
     pollint.tv_nsec = 100 * 1000000;
@@ -2244,8 +2244,8 @@ static void handle_end_loop() {
     }
 
     if (!data->prev_end_loop && bg_jobs) {
-        writestr(_(L"There are stopped or running jobs.\n"));
+        fputws(_(L"There are stopped or running jobs.\n"), stdout);
-	writestr(_(L"A second attempt to exit will force their termination.\n"));
+	fputws(_(L"A second attempt to exit will force their termination.\n"), stdout);
         reader_exit(0, 0);
         data->prev_end_loop = 1;
         return;
@@ -3260,7 +3260,7 @@ const wchar_t *reader_readline(int nchars) {
         reader_repaint_if_needed();
     }
 
-    writestr(L"\n");
+    fputc('\n', stdout);
 
     // Ensure we have no pager contents when we exit.
     if (!data->pager.empty()) {