Merge branch 'iothread_perform_lambda'

Bravely reintroducing the iothread changes that broke on g++

src/env.h

@@ -153,10 +153,10 @@ class env_vars_snapshot_t {
     std::map<wcstring, wcstring> vars;
     bool is_current() const;
 
-    env_vars_snapshot_t(const env_vars_snapshot_t &);
+public:
-    void operator=(const env_vars_snapshot_t &);
+    env_vars_snapshot_t(const env_vars_snapshot_t &) = default;
+    env_vars_snapshot_t &operator=(const env_vars_snapshot_t &) = default;
 
-   public:
     env_vars_snapshot_t(const wchar_t *const *keys);
     env_vars_snapshot_t();
 

src/highlight.cpp

@@ -327,7 +327,6 @@ static bool autosuggest_parse_command(const wcstring &buff, wcstring *out_expand
 }
 
 bool autosuggest_validate_from_history(const history_item_t &item,
-                                       file_detection_context_t &detector,
                                        const wcstring &working_directory,
                                        const env_vars_snapshot_t &vars) {
     ASSERT_IS_BACKGROUND_THREAD();
@@ -372,11 +371,7 @@ bool autosuggest_validate_from_history(const history_item_t &item,
 
     if (cmd_ok) {
         const path_list_t &paths = item.get_required_paths();
-        if (paths.empty()) {
+        suggestionOK = all_paths_are_valid(paths, working_directory);
-            suggestionOK = true;
-        } else {
-            suggestionOK = detector.paths_are_valid(paths);
-        }
     }
 
     return suggestionOK;

src/highlight.h

@@ -108,7 +108,6 @@ rgb_color_t highlight_get_color(highlight_spec_t highlight, bool is_background);
 /// specially recognizing the command. Returns true if we validated the command. If so, returns by
 /// reference whether the suggestion is valid or not.
 bool autosuggest_validate_from_history(const history_item_t &item,
-                                       file_detection_context_t &detector,
                                        const wcstring &working_directory,
                                        const env_vars_snapshot_t &vars);
 

src/history.cpp

@@ -1661,53 +1661,25 @@ void history_sanity_check() {
     // No sanity checking implemented yet...
 }
 
-int file_detection_context_t::perform_file_detection(bool test_all) {
+path_list_t valid_paths(const path_list_t &paths, const wcstring &working_directory) {
     ASSERT_IS_BACKGROUND_THREAD();
-    valid_paths.clear();
+    wcstring_list_t result;
-    // TODO: Figure out why this bothers to return a variable result since the only consumer,
+    for (const wcstring &path : paths) {
-    // perform_file_detection_done(), ignores the result. It seems like either this should always
+        if (path_is_valid(path, working_directory)) {
-    // return a constant or perform_file_detection_done() should use our return value.
+            result.push_back(path);
-    int result = 1;
-    for (path_list_t::const_iterator iter = potential_paths.begin(); iter != potential_paths.end();
-         ++iter) {
-        if (path_is_valid(*iter, working_directory)) {
-            // Push the original (possibly relative) path.
-            valid_paths.push_back(*iter);
-        } else {
-            // Not a valid path.
-            result = 0;
-            if (!test_all) break;
         }
     }
     return result;
 }
 
-bool file_detection_context_t::paths_are_valid(const path_list_t &paths) {
+bool all_paths_are_valid(const path_list_t &paths, const wcstring &working_directory) {
-    this->potential_paths = paths;
-    return perform_file_detection(false) > 0;
-}
-
-file_detection_context_t::file_detection_context_t(history_t *hist, history_identifier_t ident)
-    : history(hist), working_directory(env_get_pwd_slash()), history_item_identifier(ident) {}
-
-static int threaded_perform_file_detection(file_detection_context_t *ctx) {
     ASSERT_IS_BACKGROUND_THREAD();
-    assert(ctx != NULL);
+    for (const wcstring &path : paths) {
-    return ctx->perform_file_detection(true /* test all */);
+        if (! path_is_valid(path, working_directory)) {
-}
+            return false;
-
+        }
-static void perform_file_detection_done(file_detection_context_t *ctx, int success) {
+    }
-    UNUSED(success);
+    return true;
-    ASSERT_IS_MAIN_THREAD();
-
-    // Now that file detection is done, update the history item with the valid file paths.
-    ctx->history->set_valid_file_paths(ctx->valid_paths, ctx->history_item_identifier);
-
-    // Allow saving again.
-    ctx->history->enable_automatic_saving();
-
-    // Done with the context.
-    delete ctx;
 }
 
 static bool string_could_be_path(const wcstring &potential_path) {
@@ -1720,7 +1692,6 @@ static bool string_could_be_path(const wcstring &potential_path) {
 
 void history_t::add_pending_with_file_detection(const wcstring &str) {
     ASSERT_IS_MAIN_THREAD();
-    path_list_t potential_paths;
 
     // Find all arguments that look like they could be file paths.
     bool impending_exit = false;
@@ -1728,6 +1699,7 @@ void history_t::add_pending_with_file_detection(const wcstring &str) {
     parse_tree_from_string(str, parse_flag_none, &tree, NULL);
     size_t count = tree.size();
 
+    path_list_t potential_paths;
     for (size_t i = 0; i < count; i++) {
         const parse_node_t &node = tree.at(i);
         if (!node.has_source()) {
@@ -1764,16 +1736,18 @@ void history_t::add_pending_with_file_detection(const wcstring &str) {
         static history_identifier_t sLastIdentifier = 0;
         identifier = ++sLastIdentifier;
 
-        // Create a new detection context.
-        file_detection_context_t *context = new file_detection_context_t(this, identifier);
-        context->potential_paths.swap(potential_paths);
-
         // Prevent saving until we're done, so we have time to get the paths.
         this->disable_automatic_saving();
 
-        // Kick it off. Even though we haven't added the item yet, it updates the item on the main
+        // Check for which paths are valid on a background thread,
-        // thread, so we can't race.
+        // then on the main thread update our history item
-        iothread_perform(threaded_perform_file_detection, perform_file_detection_done, context);
+        const wcstring wd = env_get_pwd_slash();
+        iothread_perform([=](){
+            return valid_paths(potential_paths, wd);
+        }, [=](path_list_t validated_paths){
+            this->set_valid_file_paths(validated_paths, identifier);
+            this->enable_automatic_saving();
+        });
     }
 
     // Actually add the item to the history.

src/history.h

@@ -341,34 +341,12 @@ void history_destroy();
 // Perform sanity checks.
 void history_sanity_check();
 
-// A helper class for threaded detection of paths.
+// Given a list of paths and a working directory, return the paths that are valid
-struct file_detection_context_t {
+// This does disk I/O and may only be called in a background thread
-    // Constructor.
+path_list_t valid_paths(const path_list_t &paths, const wcstring &working_directory);
-    explicit file_detection_context_t(history_t *hist, history_identifier_t ident = 0);
 
-    // Determine which of potential_paths are valid, and put them in valid_paths.
+// Given a list of paths and a working directory,
-    int perform_file_detection();
+// return true if all paths in the list are valid
-
+// Returns true for if paths is empty
-    // The history associated with this context.
+bool all_paths_are_valid(const path_list_t &paths, const wcstring &working_directory);
-    history_t *const history;
-
-    // The working directory at the time the command was issued.
-    wcstring working_directory;
-
-    // Paths to test.
-    path_list_t potential_paths;
-
-    // Paths that were found to be valid.
-    path_list_t valid_paths;
-
-    // Identifier of the history item to which we are associated.
-    const history_identifier_t history_item_identifier;
-
-    // Performs file detection. Returns 1 if every path in potential_paths is valid, 0 otherwise. If
-    // test_all is true, tests every path; otherwise stops as soon as it reaches an invalid path.
-    int perform_file_detection(bool test_all);
-
-    // Determine whether the given paths are all valid.
-    bool paths_are_valid(const path_list_t &paths);
-};
 #endif

src/iothread.cpp

@@ -190,7 +190,7 @@ static void iothread_spawn() {
     VOMIT_ON_FAILURE(pthread_sigmask(SIG_SETMASK, &saved_set, NULL));
 }
 
-int iothread_perform(void_function_t &&func, void_function_t &&completion) {
+int iothread_perform_impl(void_function_t &&func, void_function_t &&completion) {
     ASSERT_IS_MAIN_THREAD();
     ASSERT_IS_NOT_FORKED_CHILD();
     iothread_init();

src/iothread.h

@@ -26,29 +26,49 @@ void iothread_service_completion(void);
 /// Waits for all iothreads to terminate.
 void iothread_drain_all(void);
 
-int iothread_perform(std::function<void(void)> &&func,
+// Internal implementation
-                     std::function<void(void)> &&completion = std::function<void(void)>());
+int iothread_perform_impl(std::function<void(void)> &&func,
+                          std::function<void(void)> &&completion);
 
-// Variant that allows computing a value in func, and then passing it to the completion handler
+// Template helpers
+// This is the glue part of the handler-completion handoff
+// In general we can just allocate an object, move the result of the handler into it,
+// and then call the completion with that object. However if our type is void,
+// this won't work (new void() fails!). So we have to use this template.
+// The type T is the return type of HANDLER and the argument to COMPLETION
 template<typename T>
-int iothread_perform(std::function<T(void)> &&handler, std::function<void(T)> &&completion) {
+struct _iothread_trampoline {
-    T *result = new T();
+    template<typename HANDLER, typename COMPLETION>
-    return iothread_perform([=](){ *result = handler(); },
+    static int perform(const HANDLER &handler, const COMPLETION &completion) {
-                            [=](){ completion(std::move(*result)); delete result; }
+        T *result = new T(); // TODO: placement new?
-                            );
+        return iothread_perform_impl([=](){ *result = handler(); },
+                                     [=](){ completion(std::move(*result)); delete result; });
+    }
+};
+
+
+// Void specialization
+template<>
+struct _iothread_trampoline<void> {
+    template<typename HANDLER, typename COMPLETION>
+    static int perform(const HANDLER &handler, const COMPLETION &completion) {
+        return iothread_perform_impl(handler, completion);
+    }
+};
+
+// iothread_perform invokes a handler on a background thread, and then a completion function
+// on the main thread. The value returned from the handler is passed to the completion.
+// In other words, this is like COMPLETION(HANDLER()) except the handler part is invoked
+// on a background thread.
+template<typename HANDLER, typename COMPLETION>
+int iothread_perform(const HANDLER &handler, const COMPLETION &completion) {
+    return _iothread_trampoline<decltype(handler())>::perform(handler, completion);
 }
 
-/// Legacy templates
+// variant of iothread_perform without a completion handler
-template <typename T>
+inline int iothread_perform(std::function<void(void)> &&func) {
-int iothread_perform(int (*handler)(T *), void (*completion)(T *, int), T *context) {
+    return iothread_perform_impl(std::move(func),
-    return iothread_perform(std::function<int(void)>([=](){return handler(context);}),
+                                 std::function<void(void)>());
-                            std::function<void(int)>([=](int v){completion(context, v);})
-                            );
-}
-
-template <typename T>
-int iothread_perform(int (*handler)(T *), T *context) {
-    return iothread_perform([=](){ handler(context); });
 }
 
 /// Performs a function on the main thread, blocking until it completes.

src/reader.cpp

@@ -1111,67 +1111,63 @@ static void completion_insert(const wchar_t *val, complete_flags_t flags) {
     reader_set_buffer_maintaining_pager(new_command_line, cursor);
 }
 
-struct autosuggestion_context_t {
+struct autosuggestion_result_t {
+    wcstring suggestion;
     wcstring search_string;
-    wcstring autosuggestion;
+};
-    size_t cursor_pos;
-    history_search_t searcher;
-    file_detection_context_t detector;
-    const wcstring working_directory;
-    const env_vars_snapshot_t vars;
-    const unsigned int generation_count;
 
-    autosuggestion_context_t(history_t *history, const wcstring &term, size_t pos)
+// Returns a function that can be invoked (potentially
-        : search_string(term),
+// on a background thread) to determine the autosuggestion
-          cursor_pos(pos),
+static std::function<autosuggestion_result_t(void)>
-          searcher(*history, term, HISTORY_SEARCH_TYPE_PREFIX),
+get_autosuggestion_performer(const wcstring &search_string, size_t cursor_pos, history_t *history) {
-          detector(history),
+    const unsigned int generation_count = s_generation_count;
-          working_directory(env_get_pwd_slash()),
+    const wcstring working_directory(env_get_pwd_slash());
-          vars(env_vars_snapshot_t::highlighting_keys),
+    env_vars_snapshot_t vars(env_vars_snapshot_t::highlighting_keys);
-          generation_count(s_generation_count) {}
+    // TODO: suspicious use of 'history' here
-
+    // This is safe because histories are immortal, but perhaps
-    // The function run in the background thread to determine an autosuggestion.
+    // this should use shared_ptr
-    int threaded_autosuggest(void) {
+    return [=]() -> autosuggestion_result_t {
         ASSERT_IS_BACKGROUND_THREAD();
 
+        const autosuggestion_result_t nothing = {};
         // If the main thread has moved on, skip all the work.
         if (generation_count != s_generation_count) {
-            return 0;
+            return nothing;
         }
 
-        VOMIT_ON_FAILURE(
+        VOMIT_ON_FAILURE(pthread_setspecific(generation_count_key,
-            pthread_setspecific(generation_count_key, (void *)(uintptr_t)generation_count));
+                                             (void *)(uintptr_t)generation_count));
 
         // Let's make sure we aren't using the empty string.
         if (search_string.empty()) {
-            return 0;
+            return nothing;
         }
 
+        history_search_t searcher(*history, search_string, HISTORY_SEARCH_TYPE_PREFIX);
         while (!reader_thread_job_is_stale() && searcher.go_backwards()) {
             history_item_t item = searcher.current_item();
 
             // Skip items with newlines because they make terrible autosuggestions.
             if (item.str().find('\n') != wcstring::npos) continue;
 
-            if (autosuggest_validate_from_history(item, detector, working_directory, vars)) {
+            if (autosuggest_validate_from_history(item, working_directory, vars)) {
                 // The command autosuggestion was handled specially, so we're done.
-                this->autosuggestion = searcher.current_string();
+                return {searcher.current_string(), search_string};
-                return 1;
             }
         }
 
         // Maybe cancel here.
-        if (reader_thread_job_is_stale()) return 0;
+        if (reader_thread_job_is_stale()) return nothing;
 
         // Here we do something a little funny. If the line ends with a space, and the cursor is not
         // at the end, don't use completion autosuggestions. It ends up being pretty weird seeing
         // stuff get spammed on the right while you go back to edit a line
         const wchar_t last_char = search_string.at(search_string.size() - 1);
-        const bool cursor_at_end = (this->cursor_pos == search_string.size());
+        const bool cursor_at_end = (cursor_pos == search_string.size());
-        if (!cursor_at_end && iswspace(last_char)) return 0;
+        if (!cursor_at_end && iswspace(last_char)) return nothing;
 
         // On the other hand, if the line ends with a quote, don't go dumping stuff after the quote.
-        if (wcschr(L"'\"", last_char) && cursor_at_end) return 0;
+        if (wcschr(L"'\"", last_char) && cursor_at_end) return nothing;
 
         // Try normal completions.
         std::vector<completion_t> completions;
@@ -1179,18 +1175,15 @@ struct autosuggestion_context_t {
         completions_sort_and_prioritize(&completions);
         if (!completions.empty()) {
             const completion_t &comp = completions.at(0);
-            size_t cursor = this->cursor_pos;
+            size_t cursor = cursor_pos;
-            this->autosuggestion = completion_apply_to_command_line(
+            wcstring suggestion = completion_apply_to_command_line(comp.completion, comp.flags,
-                comp.completion, comp.flags, this->search_string, &cursor, true /* append only */);
+                                                                   search_string, &cursor,
-            return 1;
+                                                                   true /* append only */);
+            return {std::move(suggestion), search_string};
         }
 
-        return 0;
+        return nothing;
-    }
+    };
-};
-
-static int threaded_autosuggest(autosuggestion_context_t *ctx) {
-    return ctx->threaded_autosuggest();
 }
 
 static bool can_autosuggest(void) {
@@ -1202,15 +1195,17 @@ static bool can_autosuggest(void) {
            el == &data->command_line && el->text.find_first_not_of(whitespace) != wcstring::npos;
 }
 
-static void autosuggest_completed(autosuggestion_context_t *ctx, int result) {
+// Called after an autosuggestion has been computed on a background thread
-    if (result && can_autosuggest() && ctx->search_string == data->command_line.text &&
+static void autosuggest_completed(autosuggestion_result_t result) {
-        string_prefixes_string_case_insensitive(ctx->search_string, ctx->autosuggestion)) {
+    if (! result.suggestion.empty() &&
+        can_autosuggest() &&
+        result.search_string == data->command_line.text &&
+        string_prefixes_string_case_insensitive(result.search_string, result.suggestion)) {
         // Autosuggestion is active and the search term has not changed, so we're good to go.
-        data->autosuggestion = ctx->autosuggestion;
+        data->autosuggestion = std::move(result.suggestion);
         sanity_check();
         reader_repaint();
     }
-    delete ctx;
 }
 
 static void update_autosuggestion(void) {
@@ -1220,9 +1215,8 @@ static void update_autosuggestion(void) {
     if (data->allow_autosuggestion && !data->suppress_autosuggestion &&
         !data->command_line.empty() && data->history_search.is_at_end()) {
         const editable_line_t *el = data->active_edit_line();
-        autosuggestion_context_t *ctx =
+        auto performer = get_autosuggestion_performer(el->text, el->position, data->history);
-            new autosuggestion_context_t(data->history, el->text, el->position);
+        iothread_perform(performer, &autosuggest_completed);
-        iothread_perform(threaded_autosuggest, autosuggest_completed, ctx);
     }
 }
 
@@ -2072,50 +2066,6 @@ void reader_import_history_if_necessary(void) {
     }
 }
 
-/// A class as the context pointer for a background (threaded) highlight operation.
-class background_highlight_context_t {
-   public:
-    /// The string to highlight.
-    const wcstring string_to_highlight;
-    /// Color buffer.
-    std::vector<highlight_spec_t> colors;
-    /// The position to use for bracket matching.
-    const size_t match_highlight_pos;
-    /// Function for syntax highlighting.
-    const highlight_function_t highlight_function;
-    /// Environment variables.
-    const env_vars_snapshot_t vars;
-    /// When the request was made.
-    const double when;
-    /// Gen count at the time the request was made.
-    const unsigned int generation_count;
-
-    background_highlight_context_t(const wcstring &pbuff, size_t phighlight_pos,
-                                   highlight_function_t phighlight_func)
-        : string_to_highlight(pbuff),
-          colors(pbuff.size(), 0),
-          match_highlight_pos(phighlight_pos),
-          highlight_function(phighlight_func),
-          vars(env_vars_snapshot_t::highlighting_keys),
-          when(timef()),
-          generation_count(s_generation_count) {}
-
-    int perform_highlight() {
-        if (generation_count != s_generation_count) {
-            // The gen count has changed, so don't do anything.
-            return 0;
-        }
-        VOMIT_ON_FAILURE(
-            pthread_setspecific(generation_count_key, (void *)(uintptr_t)generation_count));
-
-        if (!string_to_highlight.empty()) {
-            highlight_function(string_to_highlight, colors, match_highlight_pos, NULL /* error */,
-                               vars);
-        }
-        return 0;
-    }
-};
-
 /// Called to set the highlight flag for search results.
 static void highlight_search(void) {
     if (!data->search_buff.empty() && !data->history_search.is_at_end()) {
@@ -2131,26 +2081,46 @@ static void highlight_search(void) {
     }
 }
 
-static void highlight_complete(background_highlight_context_t *ctx, int result) {
+struct highlight_result_t {
-    UNUSED(result);  // ignored because of the indirect invocation via iothread_perform()
+    std::vector<highlight_spec_t> colors;
+    wcstring text;
+};
+
+static void highlight_complete(highlight_result_t result) {
     ASSERT_IS_MAIN_THREAD();
-    if (ctx->string_to_highlight == data->command_line.text) {
+    if (result.text == data->command_line.text) {
         // The data hasn't changed, so swap in our colors. The colors may not have changed, so do
         // nothing if they have not.
-        assert(ctx->colors.size() == data->command_line.size());
+        assert(result.colors.size() == data->command_line.size());
-        if (data->colors != ctx->colors) {
+        if (data->colors != result.colors) {
-            data->colors.swap(ctx->colors);
+            data->colors = std::move(result.colors);
             sanity_check();
             highlight_search();
             reader_repaint();
         }
     }
-
-    delete ctx;
 }
 
-static int threaded_highlight(background_highlight_context_t *ctx) {
+// Given text, bracket matching position, and whether IO is allowed,
-    return ctx->perform_highlight();
+// return a function that performs highlighting. The function may be invoked on a background thread.
+static std::function<highlight_result_t(void)> get_highlight_performer(const wcstring &text,
+                                                               long match_highlight_pos, bool no_io) {
+    env_vars_snapshot_t vars(env_vars_snapshot_t::highlighting_keys);
+    unsigned int generation_count = s_generation_count;
+    highlight_function_t highlight_func = no_io ? highlight_shell_no_io : data->highlight_function;
+    return [=]() -> highlight_result_t {
+        if (generation_count != s_generation_count) {
+            // The gen count has changed, so don't do anything.
+            return {};
+        }
+        if (text.empty()) {
+            return {};
+        }
+        VOMIT_ON_FAILURE(pthread_setspecific(generation_count_key, (void *)(uintptr_t)generation_count));
+        std::vector<highlight_spec_t> colors(text.size(), 0);
+        highlight_func(text, colors, match_highlight_pos, NULL /* error */, vars);
+        return {std::move(colors), text};
+    };
 }
 
 /// Call specified external highlighting function and then do search highlighting. Lastly, clear the
@@ -2169,16 +2139,13 @@ static void reader_super_highlight_me_plenty(int match_highlight_pos_adjust, boo
 
     reader_sanity_check();
 
-    highlight_function_t highlight_func = no_io ? highlight_shell_no_io : data->highlight_function;
+    auto highlight_performer = get_highlight_performer(el->text, match_highlight_pos, no_io);
-    background_highlight_context_t *ctx =
-        new background_highlight_context_t(el->text, match_highlight_pos, highlight_func);
     if (no_io) {
-        // Highlighting without IO, we just do it. Note that highlight_complete deletes ctx.
+        // Highlighting without IO, we just do it.
-        int result = ctx->perform_highlight();
+        highlight_complete(highlight_performer());
-        highlight_complete(ctx, result);
     } else {
         // Highlighting including I/O proceeds in the background.
-        iothread_perform(threaded_highlight, highlight_complete, ctx);
+        iothread_perform(highlight_performer, &highlight_complete);
     }
     highlight_search();