HeaderDoc code documentation improvements

Some cleanup too, move things to builtin.h from builtin.cpp that seem
to belong there.

src/builtin.cpp

@@ -71,28 +71,6 @@
 #include "wgetopt.h"
 #include "wutil.h"  // IWYU pragma: keep
 
-// The default prompt for the read command.
-#define DEFAULT_READ_PROMPT L"set_color green; echo -n read; set_color normal; echo -n \"> \""
-
-// The mode name to pass to history and input.
-#define READ_MODE_NAME L"fish_read"
-
-// The send stuff to foreground message.
-#define FG_MSG _(L"Send job %d, '%ls' to foreground\n")
-
-/// Data structure to describe a builtin.
-struct builtin_data_t {
-    // Name of the builtin.
-    const wchar_t *name;
-    // Function pointer tothe builtin implementation.
-    int (*func)(parser_t &parser, io_streams_t &streams, wchar_t **argv);
-    // Description of what the builtin does.
-    const wchar_t *desc;
-
-    bool operator<(const wcstring &) const;
-    bool operator<(const builtin_data_t *) const;
-};
-
 bool builtin_data_t::operator<(const wcstring &other) const {
     return wcscmp(this->name, other.c_str()) < 0;
 }
@@ -101,12 +79,18 @@ bool builtin_data_t::operator<(const builtin_data_t *other) const {
     return wcscmp(this->name, other->name) < 0;
 }
 
-/// Counts the number of non null pointers in the specified array.
+/// Counts the number of arguments in the specified null-terminated array
+///
+/// @param argv[]: argument list
+///
+/// @return
+///     The numer of non-NULL elements in @param *argv before the first NULL.
+///
 int builtin_count_args(const wchar_t *const *argv) {
-    int argc = 1;
+    int argc;
-    while (argv[argc] != NULL) {
+    for (argc = 1; argv[argc] != NULL; argc++);
-        argc++;
+
-    }
+    assert(argv[argc] == NULL);
     return argc;
 }
 
@@ -134,6 +118,14 @@ static int count_char(const wchar_t *str, wchar_t c) {
     return res;
 }
 
+/// Obtain help/usage information for the specified builtin from manpage in subshell
+///
+/// @param  name
+///    builtin name to get up help for
+///
+/// @return
+///    A wcstring with a formatted manpage.
+///
 wcstring builtin_help_get(parser_t &parser, io_streams_t &streams, const wchar_t *name) {
     // This won't ever work if no_exec is set.
     if (no_exec) return wcstring();
@@ -157,10 +149,12 @@ wcstring builtin_help_get(parser_t &parser, io_streams_t &streams, const wchar_t
     return out;
 }
 
-/// Print help for the specified builtin. If \c b is sb_err, also print the line information.
+/// Process and print for the specified builtin. If @c b is `sb_err`, also print the line
+/// information.
 ///
-/// If \c b is the buffer representing standard error, and the help message is about to be printed
+/// If @c b is the buffer representing standard error, and the help message is about to be printed
 /// to an interactive screen, it may be shortened to fit the screen.
+///
 void builtin_print_help(parser_t &parser, io_streams_t &streams, const wchar_t *cmd,
                         output_stream_t &b) {
     bool is_stderr = &b == &streams.err;
@@ -396,10 +390,16 @@ static int builtin_bind_add(const wchar_t *seq, const wchar_t *const *cmds, size
 
 /// Erase specified key bindings
 ///
-/// \param seq an array of all key bindings to erase
+/// @param  seq
-/// \param all if specified, _all_ key bindings will be erased
+///    an array of all key bindings to erase
-/// \param mode if specified, only bindings from that mode will be erased. If not given and \c all
+/// @param  all
-/// is \c false, \c DEFAULT_BIND_MODE will be used.
+///    if specified, _all_ key bindings will be erased
+/// @param  mode
+///    if specified, only bindings from that mode will be erased. If not given
+///    and @c all is @c false, @c DEFAULT_BIND_MODE will be used.
+/// @param  use_terminfo
+///    Whether to look use terminfo -k name
+///
 static int builtin_bind_erase(wchar_t **seq, int all, const wchar_t *mode, int use_terminfo,
                               io_streams_t &streams) {
     if (all) {
@@ -1303,7 +1303,7 @@ static bool builtin_echo_parse_numeric_sequence(const wchar_t *str, size_t *cons
 /// Bash only respects -n if it's the first argument. We'll do the same. We also support a new
 /// option -s to mean "no spaces"
 static int builtin_echo(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
-    /* Skip first arg */
+    // Skip first arg
     if (!*argv++) return STATUS_BUILTIN_ERROR;
 
     // Process options. Options must come at the beginning - the first non-option kicks us out.
@@ -1464,7 +1464,20 @@ static int builtin_pwd(parser_t &parser, io_streams_t &streams, wchar_t **argv)
     return STATUS_BUILTIN_OK;
 }
 
-/// Adds a function to the function set. It calls into function.cpp to perform any heavy lifting.
+/// Defines and adds a function to the function set. Calls into `function.cpp`
+/// to perform all heavy lifting.
+///
+/// @param  c_args
+///    The arguments. Should NOT contain 'function' as the first argument as the
+///    parser treats it as a keyword.
+/// @param  contents
+///    The function definition string
+/// @param  definition_line_offset
+///    The definition line offset
+///
+/// @return
+///    Returns 0 on success.
+///
 int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
                      const wcstring &contents, int definition_line_offset, wcstring *out_err) {
     wgetopter_t w;
@@ -2097,7 +2110,7 @@ static int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv)
         }
 
         if (buff.empty() && eof) {
-            exit_res = 1;
+            exit_res = STATUS_BUILTIN_ERROR;
         }
     }
 
@@ -2119,7 +2132,7 @@ static int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv)
                 } else {
                     env_set(argv[i], NULL, place);
                 }
-            } else {
+            } else {  // not array
                 size_t j = 0;
                 for (; i + 1 < argc; ++i) {
                     if (j < bufflen) {
@@ -2143,14 +2156,13 @@ static int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv)
                 empty = false;
             }
             env_set(argv[i], empty ? NULL : tokens.c_str(), place);
-        } else {
+        } else {  // not array
             wcstring_range loc = wcstring_range(0, 0);
 
             while (i < argc) {
                 loc = wcstring_tok(buff, (i + 1 < argc) ? ifs : wcstring(), loc);
                 env_set(argv[i], loc.first == wcstring::npos ? L"" : &buff.c_str()[loc.first],
                         place);
-
                 ++i;
             }
         }
@@ -2367,7 +2379,6 @@ static int builtin_exit(parser_t &parser, io_streams_t &streams, wchar_t **argv)
 static int builtin_cd(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     env_var_t dir_in;
     wcstring dir;
-    int res = STATUS_BUILTIN_OK;
 
     if (argv[1] == NULL) {
         dir_in = env_get_string(L"HOME");
@@ -2403,15 +2414,16 @@ static int builtin_cd(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             streams.err.append(parser.current_line());
         }
 
-        res = 1;
+        return STATUS_BUILTIN_ERROR;
-    } else if (wchdir(dir) != 0) {
+    }
+
+    if (wchdir(dir) != 0) {
         struct stat buffer;
         int status;
 
         status = wstat(dir, &buffer);
         if (!status && S_ISDIR(buffer.st_mode)) {
             streams.err.append_format(_(L"%ls: Permission denied: '%ls'\n"), argv[0], dir.c_str());
-
         } else {
             streams.err.append_format(_(L"%ls: '%ls' is not a directory\n"), argv[0], dir.c_str());
         }
@@ -2420,13 +2432,14 @@ static int builtin_cd(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             streams.err.append(parser.current_line());
         }
 
-        res = 1;
+        return STATUS_BUILTIN_ERROR;
-    } else if (!env_set_pwd()) {
-        res = 1;
-        streams.err.append_format(_(L"%ls: Could not set PWD variable\n"), argv[0]);
     }
 
-    return res;
+    if (!env_set_pwd()) {
+        streams.err.append_format(_(L"%ls: Could not set PWD variable\n"), argv[0]);
+        return STATUS_BUILTIN_ERROR;
+    } else
+        return STATUS_BUILTIN_OK;
 }
 
 /// Implementation of the builtin count command, used to count the number of arguments sent to it.
@@ -3079,7 +3092,7 @@ int builtin_fish_realpath(parser_t &parser, io_streams_t &streams, wchar_t **arg
     int argc = builtin_count_args(argv);
 
     if (argc != 2) {
-        streams.err.append_format(_(L"%ls: Expected one argument, got %d\n"), argv[0], argc - 1);
+        builtin_print_help(parser, streams, argv[0], streams.out);
         return STATUS_BUILTIN_ERROR;
     }
 
@@ -3089,7 +3102,7 @@ int builtin_fish_realpath(parser_t &parser, io_streams_t &streams, wchar_t **arg
         streams.out.append(real_path);
         free((void *)real_path);
     } else {
-        // The path isn't a simple filename and couldn't be resolved to an absolute path.
+        // We don't actually know why it failed. We should check errno
         streams.err.append_format(_(L"%ls: Invalid path: %ls\n"), argv[0], argv[1]);
         return STATUS_BUILTIN_ERROR;
     }
@@ -3164,6 +3177,14 @@ static const builtin_data_t builtin_datas[] = {
 
 #define BUILTIN_COUNT (sizeof builtin_datas / sizeof *builtin_datas)
 
+/// Look up a builtin_data_t for a specified builtin
+///
+/// @param  name
+///    Name of the builtin
+///
+/// @return
+///    Pointer to a builtin_data_t
+///
 static const builtin_data_t *builtin_lookup(const wcstring &name) {
     const builtin_data_t *array_end = builtin_datas + BUILTIN_COUNT;
     const builtin_data_t *found = std::lower_bound(builtin_datas, array_end, name);
@@ -3173,35 +3194,37 @@ static const builtin_data_t *builtin_lookup(const wcstring &name) {
     return NULL;
 }
 
+/// Initialize builtin data.
 void builtin_init() {
     for (size_t i = 0; i < BUILTIN_COUNT; i++) {
         intern_static(builtin_datas[i].name);
     }
 }
 
+/// Destroy builtin data.
 void builtin_destroy() {}
 
-int builtin_exists(const wcstring &cmd) { return !!builtin_lookup(cmd); }
+/// Is there a builtin command with the given name?
+bool builtin_exists(const wcstring &cmd) { return !!builtin_lookup(cmd); }
 
-/// Return true if the specified builtin should handle it's own help, false otherwise.
+/// If builtin takes care of printing help itself
-static int internal_help(const wchar_t *cmd) {
+static bool builtin_handles_help(const wchar_t *cmd) {
     CHECK(cmd, 0);
     return contains(cmd, L"for", L"while", L"function", L"if", L"end", L"switch", L"case", L"count",
                     L"printf");
 }
 
+/// Execute a builtin command
 int builtin_run(parser_t &parser, const wchar_t *const *argv, io_streams_t &streams) {
     int (*cmd)(parser_t & parser, io_streams_t & streams, const wchar_t *const *argv) = NULL;
-
+    if (argv == NULL || argv[0] == NULL) return STATUS_BUILTIN_ERROR;
-    CHECK(argv, STATUS_BUILTIN_ERROR);
-    CHECK(argv[0], STATUS_BUILTIN_ERROR);
 
     const builtin_data_t *data = builtin_lookup(argv[0]);
     cmd = (int (*)(parser_t & parser, io_streams_t & streams, const wchar_t *const *))(
         data ? data->func : NULL);
 
-    if (argv[1] != 0 && !internal_help(argv[0])) {
+    if (argv[1] != NULL && !builtin_handles_help(argv[0])) {
-        if (argv[2] == 0 && (parse_util_argument_is_help(argv[1], 0))) {
+        if (argv[2] == NULL && (parse_util_argument_is_help(argv[1], 0))) {
             builtin_print_help(parser, streams, argv[0], streams.out);
             return STATUS_BUILTIN_OK;
         }
@@ -3215,6 +3238,7 @@ int builtin_run(parser_t &parser, const wchar_t *const *argv, io_streams_t &stre
     return STATUS_BUILTIN_ERROR;
 }
 
+/// Returns a list of all builtin names.
 wcstring_list_t builtin_get_names(void) {
     wcstring_list_t result;
     result.reserve(BUILTIN_COUNT);
@@ -3224,6 +3248,7 @@ wcstring_list_t builtin_get_names(void) {
     return result;
 }
 
+/// Insert all builtin names into list.
 void builtin_get_names(std::vector<completion_t> *list) {
     assert(list != NULL);
     list->reserve(list->size() + BUILTIN_COUNT);
@@ -3232,6 +3257,7 @@ void builtin_get_names(std::vector<completion_t> *list) {
     }
 }
 
+/// Return a one-line description of the specified builtin.
 wcstring builtin_get_desc(const wcstring &name) {
     wcstring result;
     const builtin_data_t *builtin = builtin_lookup(name);

src/builtin.h

@@ -12,79 +12,86 @@ class parser_t;
 class output_stream_t;
 struct io_streams_t;
 
+/// Data structure to describe a builtin.
+struct builtin_data_t {
+    // Name of the builtin.
+    const wchar_t *name;
+    // Function pointer tothe builtin implementation.
+    int (*func)(parser_t &parser, io_streams_t &streams, wchar_t **argv);
+    // Description of what the builtin does.
+    const wchar_t *desc;
+
+    bool operator<(const wcstring &) const;
+    bool operator<(const builtin_data_t *) const;
+};
+
+/// The default prompt for the read command.
+#define DEFAULT_READ_PROMPT L"set_color green; echo -n read; set_color normal; echo -n \"> \""
+
+/// The mode name to pass to history and input.
+#define READ_MODE_NAME L"fish_read"
+
 enum { COMMAND_NOT_BUILTIN, BUILTIN_REGULAR, BUILTIN_FUNCTION };
 
-// Error message on missing argument.
+/// Error message on missing argument.
 #define BUILTIN_ERR_MISSING _(L"%ls: Expected argument for option %ls\n")
 
-// Error message on invalid combination of options.
+/// Error message on invalid combination of options.
 #define BUILTIN_ERR_COMBO _(L"%ls: Invalid combination of options\n")
 
-// Error message on invalid combination of options.
+/// Error message on invalid combination of options.
 #define BUILTIN_ERR_COMBO2 _(L"%ls: Invalid combination of options,\n%ls\n")
 
-// Error message on multiple scope levels for variables.
+/// Error message on multiple scope levels for variables.
 #define BUILTIN_ERR_GLOCAL \
     _(L"%ls: Variable scope can only be one of universal, global and local\n")
 
-// Error message for specifying both export and unexport to set/read.
+/// Error message for specifying both export and unexport to set/read.
 #define BUILTIN_ERR_EXPUNEXP _(L"%ls: Variable can't be both exported and unexported\n")
 
-// Error message for unknown switch.
+/// Error message for unknown switch.
 #define BUILTIN_ERR_UNKNOWN _(L"%ls: Unknown option '%ls'\n")
 
-// Error message for unexpected args.
+/// Error message for unexpected args.
 #define BUILTIN_ERR_ARG_COUNT _(L"%ls: %ls command expected %d args, got %d\n")
 
-// Error message for invalid character in variable name.
+/// Error message for invalid character in variable name.
 #define BUILTIN_ERR_VARCHAR                                                                 \
     _(L"%ls: Invalid character '%lc' in variable name. Only alphanumerical characters and " \
       L"underscores are valid in a variable name.\n")
 
-// Error message for invalid (empty) variable name.
+/// Error message for invalid (empty) variable name.
 #define BUILTIN_ERR_VARNAME_ZERO _(L"%ls: Variable name can not be the empty string\n")
 
-// Error message when too many arguments are supplied to a builtin.
+/// Error message when too many arguments are supplied to a builtin.
 #define BUILTIN_ERR_TOO_MANY_ARGUMENTS _(L"%ls: Too many arguments\n")
 
+/// Error message when number expected
 #define BUILTIN_ERR_NOT_NUMBER _(L"%ls: Argument '%ls' is not a number\n")
 
-// Initialize builtin data.
+/// The send stuff to foreground message.
+#define FG_MSG _(L"Send job %d, '%ls' to foreground\n")
+
 void builtin_init();
-
-// Destroy builtin data.
 void builtin_destroy();
+bool builtin_exists(const wcstring &cmd);
 
-// Is there a builtin command with the given name?
-int builtin_exists(const wcstring &cmd);
-
-// Execute a builtin command
-//
-// \param parser The parser being used
-// \param argv Array containing the command and parameters of the builtin.  The list is terminated
-//   by a null pointer. This syntax resembles the syntax for exec.
-// \param io the io redirections to perform on this builtin.
-//
-// \return the exit status of the builtin command
 int builtin_run(parser_t &parser, const wchar_t *const *argv, io_streams_t &streams);
 
-// Returns a list of all builtin names.
 wcstring_list_t builtin_get_names();
-
-// Insert all builtin names into list.
 void builtin_get_names(std::vector<completion_t> *list);
-
-// Return a one-line description of the specified builtin.
 wcstring builtin_get_desc(const wcstring &b);
 
-// Support for setting and removing transient command lines. This is used by 'complete -C' in order
+/// Support for setting and removing transient command lines. This is used by
-// to make the commandline builtin operate on the string to complete instead of operating on
+/// 'complete -C' in order to make the commandline builtin operate on the string
-// whatever is to be completed. It's also used by completion wrappers, to allow a command to appear
+/// to complete instead of operating on whatever is to be completed. It's also
-// as the command being wrapped for the purposes of completion.
+/// used by completion wrappers, to allow a command to appear as the command
-//
+/// being wrapped for the purposes of completion.
-// Instantiating an instance of builtin_commandline_scoped_transient_t pushes the command as the new
+///
-// transient commandline. The destructor removes it. It will assert if construction/destruction does
+/// Instantiating an instance of builtin_commandline_scoped_transient_t pushes
-// not happen in a stack-like (LIFO) order.
+/// the command as the new transient commandline. The destructor removes it. It
+/// will assert if construction/destruction does not happen in a stack-like
+/// (LIFO) order.
 class builtin_commandline_scoped_transient_t {
     size_t token;
 
@@ -93,30 +100,20 @@ class builtin_commandline_scoped_transient_t {
     ~builtin_commandline_scoped_transient_t();
 };
 
-// Run the __fish_print_help function to obtain the help information for the specified command.
 wcstring builtin_help_get(parser_t &parser, const wchar_t *cmd);
 
-// Defines a function. Returns 0 on success. args should NOT contain 'function' as the first
-// argument as the parser treats it as a keyword.
 int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_list_t &c_args,
                      const wcstring &contents, int definition_line_offset, wcstring *out_err);
 
-// Print help for the specified builtin. If \c b is sb_err, also print the line information.
 void builtin_print_help(parser_t &parser, io_streams_t &streams, const wchar_t *cmd,
                         output_stream_t &b);
-
-// Counts the number of non null pointers in the specified array.
 int builtin_count_args(const wchar_t *const *argv);
 
-// Perform error reporting for encounter with unknown option.
 void builtin_unknown_option(parser_t &parser, io_streams_t &streams, const wchar_t *cmd,
                             const wchar_t *opt);
 
-// Perform error reporting for encounter with missing argument.
 void builtin_missing_argument(parser_t &parser, io_streams_t &streams, const wchar_t *cmd,
                               const wchar_t *opt);
 
-// This function works like wperror, but it prints its result into the streams.err string instead
-// to stderr. Used by the builtin commands.
 void builtin_wperror(const wchar_t *s, io_streams_t &streams);
 #endif

src/complete.cpp

@@ -123,7 +123,6 @@ typedef struct complete_entry_opt {
     }
 
 } complete_entry_opt_t;
-
 // Last value used in the order field of completion_entry_t.
 static unsigned int kCompleteOrder = 0;
 
@@ -375,7 +374,6 @@ void append_completion(std::vector<completion_t> *completions, const wcstring &c
     // Nasty hack for #1241 - since the constructor needs the completion string to resolve
     // AUTO_SPACE, and we aren't providing it with the completion, we have to do the resolution
     // ourselves. We should get this resolving out of the constructor.
-    assert(completions != NULL);
     const wcstring empty;
     completions->push_back(completion_t(empty, empty, match, resolve_auto_space(comp, flags)));
     completion_t *last = &completions->back();
@@ -821,7 +819,7 @@ static bool short_ok(const wcstring &arg, const complete_entry_opt_t *entry,
     return result;
 }
 
-// Load command-specific completions for the specified command.
+/// Load command-specific completions for the specified command.
 static void complete_load(const wcstring &name, bool reload) {
     // We have to load this as a function, since it may define a --wraps or signature.
     // See issue #2466.
@@ -829,9 +827,8 @@ static void complete_load(const wcstring &name, bool reload) {
     completion_autoloader.load(name, reload);
 }
 
-// Performed on main thread, from background thread. Return type is ignored.
+/// Performed on main thread, from background thread. Return type is ignored.
 static int complete_load_no_reload(wcstring *name) {
-    assert(name != NULL);
     ASSERT_IS_MAIN_THREAD();
     complete_load(*name, false);
     return 0;

src/expand.cpp

@@ -1603,7 +1603,7 @@ bool expand_abbreviation(const wcstring &src, wcstring *output) {
 
         // See if this command matches.
         if (line.compare(0, cmd_end, src) == 0) {
-            // Success. Set output to everythign past the end of the string.
+            // Success. Set output to everything past the end of the string.
             if (output != NULL) output->assign(line, separator + 1, wcstring::npos);
 
             result = true;

src/screen.cpp

@@ -223,9 +223,7 @@ size_t escape_code_length(const wchar_t *code) {
                 }
             }
         }
-    }
 
-    if (cur_term != NULL) {
         // Detect these semi-common terminfo escapes without any parameter values, all of which
         // don't move the cursor.
         char *const esc2[] = {enter_bold_mode,        exit_attribute_mode,   enter_underline_mode,
@@ -1039,7 +1037,6 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro
              const int *indent, size_t cursor_pos, const page_rendering_t &pager,
              bool cursor_is_within_pager) {
     screen_data_t::cursor_t cursor_arr;
-
     CHECK(s, );
     CHECK(indent, );
 
@@ -1126,7 +1123,6 @@ void s_write(screen_t *s, const wcstring &left_prompt, const wcstring &right_pro
     s_update(s, layout.left_prompt.c_str(), layout.right_prompt.c_str());
     s_save_status(s);
 }
-
 void s_reset(screen_t *s, screen_reset_mode_t mode) {
     CHECK(s, );