From f500ecaffbc22db40a2b8d1ee0f5458a0d75ced1 Mon Sep 17 00:00:00 2001 From: Strahinja Val Markovic Date: Sun, 12 Aug 2012 19:01:39 -0700 Subject: [PATCH] Fix for hang on insert enter after "foo.bar" If the user had code like "foo.bar" and then entered insert mode after the 'r' in "bar", YCM would cause vim to hang. The problem happened because a sorting task was created that would try to sort on the latest clang result but none would be created because a clang task was not created in this occasion. clang_data_ready_ would remain false and would never be set to true, thus causing an infinite loop in SortingThreadMain since the thread would forever wait on the mutex. This was rectified with better handling of the clang results cache. Now the cache is a full class and it also stores the line & column number of the location for which the results were computed. Better logic is in place for the cache invalidation. --- cpp/ycm/ClangCompleter.cpp | 98 ++++++++++++++++-------- cpp/ycm/ClangCompleter.h | 35 ++++++--- cpp/ycm/ClangResultsCache.cpp | 45 +++++++++++ cpp/ycm/ClangResultsCache.h | 83 ++++++++++++++++++++ python/completers/cpp/clang_completer.py | 1 + 5 files changed, 219 insertions(+), 43 deletions(-) create mode 100644 cpp/ycm/ClangResultsCache.cpp create mode 100644 cpp/ycm/ClangResultsCache.h diff --git a/cpp/ycm/ClangCompleter.cpp b/cpp/ycm/ClangCompleter.cpp index 84c4676f..b345306e 100644 --- a/cpp/ycm/ClangCompleter.cpp +++ b/cpp/ycm/ClangCompleter.cpp @@ -18,9 +18,9 @@ #include "ClangCompleter.h" #include "Candidate.h" #include "TranslationUnit.h" -#include "CompletionData.h" #include "standard.h" #include "CandidateRepository.h" +#include "CompletionData.h" #include "ConcurrentLatestValue.h" #include "Utils.h" #include "ClangUtils.h" @@ -28,9 +28,6 @@ #include #include -// TODO: remove all explicit uses of the boost:: prefix by adding explicit using -// directives for the stuff we need -namespace fs = boost::filesystem; using boost::bind; using boost::cref; using boost::function; @@ -52,9 +49,6 @@ using boost::unordered_map; namespace YouCompleteMe { -typedef function< std::vector< CompletionData >() > - FunctionReturnsCompletionDataVector; - extern const unsigned int MAX_ASYNC_THREADS; extern const unsigned int MIN_ASYNC_THREADS; @@ -186,8 +180,6 @@ Future< void > ClangCompleter::UpdateTranslationUnitAsync( shared_ptr< ClangPackagedTask > clang_packaged_task = make_shared< ClangPackagedTask >(); - // TODO: vim hangs when we just open it and try to enter insert mode after the - // last '_' in ->parsing_task_ here clang_packaged_task->parsing_task_ = packaged_task< void >( functor ); unique_future< void > future = clang_packaged_task->parsing_task_.get_future(); @@ -217,18 +209,21 @@ ClangCompleter::CandidatesForLocationInFile( Future< AsyncCompletions > ClangCompleter::CandidatesForQueryAndLocationInFileAsync( - std::string query, - std::string filename, + const std::string &query, + const std::string &filename, int line, int column, - std::vector< UnsavedFile > unsaved_files, - std::vector< std::string > flags ) + const std::vector< UnsavedFile > &unsaved_files, + const std::vector< std::string > &flags ) { // TODO: throw exception when threading is not enabled and this is called if ( !threading_enabled_ ) return Future< AsyncCompletions >(); - if ( query.empty() ) + bool skip_clang_result_cache = ShouldSkipClangResultCache( query, + line, + column ); + if ( skip_clang_result_cache ) { // The clang thread is busy, return nothing if ( UpdatingTranslationUnit( filename ) ) @@ -248,25 +243,71 @@ ClangCompleter::CandidatesForQueryAndLocationInFileAsync( // the sorting task needs to be set before the clang task (if any) just in // case the clang task finishes (and therefore notifies a sorting thread to // consume a sorting task) before the sorting task is set + unique_future< AsyncCompletions > future = CreateSortingTask( query ); - FunctionReturnsCompletionDataVector sort_candidates_for_query_functor = + if ( skip_clang_result_cache ) + { + CreateClangTask( filename, line, column, unsaved_files, flags ); + } + + return Future< AsyncCompletions >( boost::move( future ) ); +} + + +bool ClangCompleter::ShouldSkipClangResultCache( const std::string &query, + int line, + int column ) +{ + // We need query.empty() in addition to the second check because if we don't + // have it, then we have a problem in the following situation: + // The user has a variable 'foo' of type 'A' and a variable 'bar' of type 'B'. + // He then types in 'foo.' and we store the clang results and also return + // them. The user then deletes that code and types in 'bar.'. Without the + // query.empty() check we would return the results from the cache here (it's + // the same line and column!) which would be incorrect. + return query.empty() || latest_clang_results_ + .NewPositionDifferentFromStoredPosition( line, column ); +} + + +unique_future< AsyncCompletions > ClangCompleter::CreateSortingTask( + const std::string &query ) +{ + // Careful! The code in this function may burn your eyes. + + function< CompletionDatas( const CompletionDatas& ) > + sort_candidates_for_query_functor = bind( &ClangCompleter::SortCandidatesForQuery, boost::ref( *this ), query, - boost::cref( latest_clang_results_ ) ); + _1 ); + + function< CompletionDatas() > operate_on_completion_data_functor = + bind( &ClangResultsCache::OperateOnCompletionDatas< CompletionDatas >, + boost::cref( latest_clang_results_ ), + boost::move( sort_candidates_for_query_functor ) ); shared_ptr< packaged_task< AsyncCompletions > > task = make_shared< packaged_task< AsyncCompletions > >( bind( ReturnValueAsShared< std::vector< CompletionData > >, - sort_candidates_for_query_functor ) ); + boost::move( operate_on_completion_data_functor ) ) ); unique_future< AsyncCompletions > future = task->get_future(); sorting_task_.Set( task ); + return future; +} - if ( query.empty() ) - { - FunctionReturnsCompletionDataVector - candidates_for_location_functor = + +void ClangCompleter::CreateClangTask( + std::string filename, + int line, + int column, + std::vector< UnsavedFile > unsaved_files, + std::vector< std::string > flags ) +{ + latest_clang_results_.ResetWithNewLineAndColumn( line, column ); + + function< CompletionDatas() > candidates_for_location_functor = bind( &ClangCompleter::CandidatesForLocationInFile, boost::ref( *this ), boost::move( filename ), @@ -283,9 +324,6 @@ ClangCompleter::CandidatesForQueryAndLocationInFileAsync( candidates_for_location_functor ) ); clang_task_.Set( clang_packaged_task ); - } - - return Future< AsyncCompletions >( boost::move( future ) ); } @@ -396,11 +434,7 @@ void ClangCompleter::ClangThreadMain() unique_future< AsyncCompletions > future = task->completions_task_.get_future(); - - { - unique_lock< shared_mutex > writer_lock( latest_clang_results_mutex_ ); - latest_clang_results_ = *future.get(); - } + latest_clang_results_.SetCompletionDatas( *future.get() ); { lock_guard< mutex > lock( clang_data_ready_mutex_ ); @@ -439,11 +473,7 @@ void ClangCompleter::SortingThreadMain() shared_ptr< packaged_task< AsyncCompletions > > task = sorting_task_.Get(); - { - shared_lock< shared_mutex > reader_lock( latest_clang_results_mutex_ ); - - ( *task )(); - } + ( *task )(); } catch ( thread_interrupted& ) diff --git a/cpp/ycm/ClangCompleter.h b/cpp/ycm/ClangCompleter.h index 1ba0d624..423498d5 100644 --- a/cpp/ycm/ClangCompleter.h +++ b/cpp/ycm/ClangCompleter.h @@ -22,8 +22,10 @@ #include "Future.h" #include "UnsavedFile.h" #include "Diagnostic.h" +#include "ClangResultsCache.h" #include +#include #include #include @@ -38,7 +40,9 @@ class CandidateRepository; class TranslationUnit; struct CompletionData; -typedef boost::shared_ptr< std::vector< CompletionData > > AsyncCompletions; +typedef std::vector< CompletionData > CompletionDatas; + +typedef boost::shared_ptr< CompletionDatas > AsyncCompletions; typedef boost::unordered_map< std::string, boost::shared_ptr< @@ -82,15 +86,13 @@ public: const std::vector< UnsavedFile > &unsaved_files, const std::vector< std::string > &flags ); - // NOTE: params are taken by value on purpose! With a C++11 compiler we can - // avoid internal copies if params are taken by value (move ctors FTW) Future< AsyncCompletions > CandidatesForQueryAndLocationInFileAsync( - std::string query, - std::string filename, + const std::string &query, + const std::string &filename, int line, int column, - std::vector< UnsavedFile > unsaved_files, - std::vector< std::string > flags ); + const std::vector< UnsavedFile > &unsaved_files, + const std::vector< std::string > &flags ); private: @@ -109,6 +111,22 @@ private: typedef ConcurrentLatestValue< boost::shared_ptr< ClangPackagedTask > > LatestClangTask; + bool ShouldSkipClangResultCache( const std::string &query, + int line, + int column ); + + boost::unique_future< AsyncCompletions > CreateSortingTask( + const std::string &query ); + + // NOTE: params are taken by value on purpose! With a C++11 compiler we can + // avoid internal copies if params are taken by value (move ctors FTW) + void CreateClangTask( + std::string filename, + int line, + int column, + std::vector< UnsavedFile > unsaved_files, + std::vector< std::string > flags ); + boost::shared_ptr< TranslationUnit > GetTranslationUnitForFile( const std::string &filename, const std::vector< UnsavedFile > &unsaved_files, @@ -146,8 +164,7 @@ private: boost::mutex clang_data_ready_mutex_; boost::condition_variable clang_data_ready_condition_variable_; - std::vector< CompletionData > latest_clang_results_; - boost::shared_mutex latest_clang_results_mutex_; + ClangResultsCache latest_clang_results_; // Unfortunately clang is not thread-safe so we need to be careful when we // access it. Only one thread at a time is allowed to access any single diff --git a/cpp/ycm/ClangResultsCache.cpp b/cpp/ycm/ClangResultsCache.cpp new file mode 100644 index 00000000..604c476b --- /dev/null +++ b/cpp/ycm/ClangResultsCache.cpp @@ -0,0 +1,45 @@ +// Copyright (C) 2011, 2012 Strahinja Val Markovic +// +// This file is part of YouCompleteMe. +// +// YouCompleteMe is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// YouCompleteMe is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with YouCompleteMe. If not, see . + +#include "ClangResultsCache.h" +#include "standard.h" + +using boost::shared_mutex; +using boost::shared_lock; +using boost::unique_lock; + +namespace YouCompleteMe +{ + +bool ClangResultsCache::NewPositionDifferentFromStoredPosition( int new_line, + int new_colum ) + const +{ + shared_lock< shared_mutex > reader_lock( access_mutex_ ); + return line_ != new_line || column_ != new_colum; +} + +void ClangResultsCache::ResetWithNewLineAndColumn( int new_line, int new_colum ) +{ + unique_lock< shared_mutex > reader_lock( access_mutex_ ); + + line_ = new_line; + column_ = new_colum; + completion_datas_.clear(); +} + +} // namespace YouCompleteMe diff --git a/cpp/ycm/ClangResultsCache.h b/cpp/ycm/ClangResultsCache.h new file mode 100644 index 00000000..e1dba0b7 --- /dev/null +++ b/cpp/ycm/ClangResultsCache.h @@ -0,0 +1,83 @@ +// Copyright (C) 2011, 2012 Strahinja Val Markovic +// +// This file is part of YouCompleteMe. +// +// YouCompleteMe is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// YouCompleteMe is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with YouCompleteMe. If not, see . + +#ifndef CLANGRESULTSCACHE_H_REUWM3RU +#define CLANGRESULTSCACHE_H_REUWM3RU + +#include "CompletionData.h" + +#include +#include +#include +#include +#include + +namespace YouCompleteMe +{ + +struct CompletionData; + +class ClangResultsCache +{ +public: + + ClangResultsCache() : line_( -1 ), column_( -1 ) {} + + bool NewPositionDifferentFromStoredPosition( int new_line, int new_colum ) + const; + + void ResetWithNewLineAndColumn( int new_line, int new_colum ); + + void SetCompletionDatas( const std::vector< CompletionData > new_completions ) + { + completion_datas_ = new_completions; + } + +#ifndef BOOST_NO_RVALUE_REFERENCES +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wc++98-compat" + + void SetCompletionDatas( std::vector< CompletionData >&& new_completions ) + { + completion_datas_ = new_completions; + } + +#pragma clang diagnostic pop +#endif //#ifndef BOOST_NO_RVALUE_REFERENCES + + template< typename T > + T OperateOnCompletionDatas( + boost::function< T( const std::vector< CompletionData >& ) > operation ) + const + { + boost::shared_lock< boost::shared_mutex > reader_lock( access_mutex_ ); + return operation( completion_datas_ ); + } + +private: + + int line_; + int column_; + std::vector< CompletionData > completion_datas_; + + mutable boost::shared_mutex access_mutex_; +}; + +} // namespace YouCompleteMe + +#endif /* end of include guard: CLANGRESULTSCACHE_H_REUWM3RU */ + diff --git a/python/completers/cpp/clang_completer.py b/python/completers/cpp/clang_completer.py index 8b5155fb..a1913699 100644 --- a/python/completers/cpp/clang_completer.py +++ b/python/completers/cpp/clang_completer.py @@ -124,6 +124,7 @@ class ClangCompleter( Completer ): return self.parse_future.ResultsReady() + def GetDiagnosticsForCurrentFile( self ): if self.DiagnosticsForCurrentFileReady(): self.last_diagnostics = [ DiagnosticToDict( x ) for x in