Ensuring TUs expire when compilation flags change.

cpp/ycm/ClangCompleter/TranslationUnitStore.cpp

@@ -21,6 +21,7 @@
 #include "exceptions.h"
 
 #include <boost/thread/locks.hpp>
+#include <boost/functional/hash.hpp>
 
 using boost::lock_guard;
 using boost::shared_ptr;
@@ -29,6 +30,14 @@ using boost::mutex;
 
 namespace YouCompleteMe {
 
+namespace {
+
+std::size_t HashForFlags( const std::vector< std::string > &flags ) {
+  return boost::hash< std::vector< std::string > >()( flags );
+}
+
+}  // unnamed namespace
+
 TranslationUnitStore::TranslationUnitStore( CXIndex clang_index )
   : clang_index_( clang_index ) {
 }
@@ -53,11 +62,13 @@ shared_ptr< TranslationUnit > TranslationUnitStore::GetOrCreate(
   bool &translation_unit_created ) {
   translation_unit_created = false;
   {
-    lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );
+    lock_guard< mutex > lock( filename_to_translation_unit_and_flags_mutex_ );
     shared_ptr< TranslationUnit > current_unit = GetNoLock( filename );
 
-    if ( current_unit )
+    if ( current_unit &&
+         HashForFlags( flags ) == filename_to_flags_hash_[ filename ] ) {
       return current_unit;
+    }
 
     // We create and store an invalid, sentinel TU so that other threads don't
     // try to create a TU for the same file while we are trying to create this
@@ -65,6 +76,10 @@ shared_ptr< TranslationUnit > TranslationUnitStore::GetOrCreate(
     // with the valid object.
     filename_to_translation_unit_[ filename ] =
         make_shared< TranslationUnit >();
+
+    // We need to store the flags for the sentinel TU so that other threads end
+    // up returning the sentinel TU while the real one is being created.
+    filename_to_flags_hash_[ filename ] = HashForFlags( flags );
   }
 
   shared_ptr< TranslationUnit > unit;
@@ -75,13 +90,14 @@ shared_ptr< TranslationUnit > TranslationUnitStore::GetOrCreate(
                                            flags,
                                            clang_index_ );
   } catch ( ClangParseError & ) {
-    Erase( filename_to_translation_unit_, filename );
+    Remove( filename );
     return unit;
   }
 
   {
-    lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );
+    lock_guard< mutex > lock( filename_to_translation_unit_and_flags_mutex_ );
     filename_to_translation_unit_[ filename ] = unit;
+    // Flags have already been stored.
   }
 
   translation_unit_created = true;
@@ -90,18 +106,20 @@ shared_ptr< TranslationUnit > TranslationUnitStore::GetOrCreate(
 
 shared_ptr< TranslationUnit > TranslationUnitStore::Get(
     const std::string &filename ) {
-  lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );
+  lock_guard< mutex > lock( filename_to_translation_unit_and_flags_mutex_ );
   return GetNoLock( filename );
 }
 
 bool TranslationUnitStore::Remove( const std::string &filename ) {
-  lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );
+  lock_guard< mutex > lock( filename_to_translation_unit_and_flags_mutex_ );
+  Erase( filename_to_flags_hash_, filename );
   return Erase( filename_to_translation_unit_, filename );
 }
 
 void TranslationUnitStore::RemoveAll() {
-  lock_guard< mutex > lock( filename_to_translation_unit_mutex_ );
+  lock_guard< mutex > lock( filename_to_translation_unit_and_flags_mutex_ );
   filename_to_translation_unit_.clear();
+  filename_to_flags_hash_.clear();
 }
 
 shared_ptr< TranslationUnit > TranslationUnitStore::GetNoLock(

cpp/ycm/ClangCompleter/TranslationUnitStore.h

@@ -38,6 +38,8 @@ public:
   TranslationUnitStore( CXIndex clang_index );
   ~TranslationUnitStore();
 
+  // You can even call this function for the same filename from multiple
+  // threads; the TU store will ensure only one TU is created.
   boost::shared_ptr< TranslationUnit > GetOrCreate(
     const std::string &filename,
     const std::vector< UnsavedFile > &unsaved_files,
@@ -49,6 +51,10 @@ public:
     const std::vector< std::string > &flags,
     bool &translation_unit_created );
 
+  // Careful here! While GetOrCreate makes sure to take into account the flags
+  // for the file before returning a stored TU (if the flags changed, the TU is
+  // not really valid anymore and a new one should be built), this function does
+  // not. You might end up getting a stale TU.
   boost::shared_ptr< TranslationUnit > Get( const std::string &filename );
 
   bool Remove( const std::string &filename );
@@ -57,16 +63,20 @@ public:
 
 private:
 
-  // WARNING: This does accesses filename_to_translation_unit_ without a lock!
+  // WARNING: This accesses filename_to_translation_unit_ without a lock!
   boost::shared_ptr< TranslationUnit > GetNoLock( const std::string &filename );
 
 
   typedef boost::unordered_map< std::string,
           boost::shared_ptr< TranslationUnit > > TranslationUnitForFilename;
 
+  typedef boost::unordered_map< std::string,
+          std::size_t > FlagsHashForFilename;
+
   CXIndex clang_index_;
   TranslationUnitForFilename filename_to_translation_unit_;
-  boost::mutex filename_to_translation_unit_mutex_;
+  FlagsHashForFilename filename_to_flags_hash_;
+  boost::mutex filename_to_translation_unit_and_flags_mutex_;
 };
 
 } // namespace YouCompleteMe