[cfe-commits] r159256 - in /cfe/trunk: include/clang/Basic/FileManager.h lib/Basic/FileManager.cpp

Chandler Carruth chandlerc at google.com
Wed Jun 27 02:41:39 PDT 2012


This thread seems fine to discuss it -- I didn't want to have missed an
existing discussion.

I don't care about backing this out -- it's not breaking anything. I just
have reservations about the interface and strategy as a whole. Let's just
talk about it.

On Wed, Jun 27, 2012 at 2:17 AM, Axel Naumann <Axel.Naumann at cern.ch
>> <mailto:Axel.Naumann at cern.ch>> wrote:
>>
>>    Author: axel
>>    Date: Wed Jun 27 04:17:42 2012
>>    New Revision: 159256
>>
>>    URL: http://llvm.org/viewvc/llvm-**project?rev=159256&view=rev<http://llvm.org/viewvc/llvm-project?rev=159256&view=rev>
>>    Log:
>>     >From Vassil Vassilev:
>>    add interface for removing a FileEntry from the cache.
>>    Forces a re-read the contents from disk, e.g. because a tool (like
>>    cling) wants to pick up a modified file.
>>
>>    Modified:
>>        cfe/trunk/include/clang/Basic/**FileManager.h
>>        cfe/trunk/lib/Basic/**FileManager.cpp
>>
>>    Modified: cfe/trunk/include/clang/Basic/**FileManager.h
>>    URL:
>>    http://llvm.org/viewvc/llvm-**project/cfe/trunk/include/**
>> clang/Basic/FileManager.h?rev=**159256&r1=159255&r2=159256&**view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=159256&r1=159255&r2=159256&view=diff>
>>    ==============================**==============================**
>> ==================
>>    --- cfe/trunk/include/clang/Basic/**FileManager.h (original)
>>    +++ cfe/trunk/include/clang/Basic/**FileManager.h Wed Jun 27 04:17:42
>> 2012
>>    @@ -222,6 +222,9 @@
>>       /// FileManager's FileSystemOptions.
>>       bool getNoncachedStatValue(**StringRef Path, struct stat &StatBuf);
>>
>>    +  /// \brief Remove the real file Entry from the cache.
>>    +  void InvalidateCache(const FileEntry* Entry);
>>    +
>>
>
Please follow the LLVM coding conventions, especially if you're doing
post-commit review. ;] Methods are 'camelCase' and the '*' attaches to the
variable or parameter name.


>       /// \brief If path is not absolute and FileSystemOptions set the
>>    working
>>       /// directory, the path is modified to be relative to the given
>>       /// working directory.
>>
>>    Modified: cfe/trunk/lib/Basic/**FileManager.cpp
>>    URL:
>>    http://llvm.org/viewvc/llvm-**project/cfe/trunk/lib/Basic/**
>> FileManager.cpp?rev=159256&r1=**159255&r2=159256&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=159256&r1=159255&r2=159256&view=diff>
>>    ==============================**==============================**
>> ==================
>>    --- cfe/trunk/lib/Basic/**FileManager.cpp (original)
>>    +++ cfe/trunk/lib/Basic/**FileManager.cpp Wed Jun 27 04:17:42 2012
>>    @@ -111,6 +111,8 @@
>>       }
>>
>>       size_t size() const { return UniqueFiles.size(); }
>>    +
>>    +  friend class FileManager;
>>
>
Adding these friends seems extremely unfortunate. Why are they required?


>      };
>>
>>      //===-------------------------**------------------------------**
>> ---------------===//
>>    @@ -152,6 +154,8 @@
>>       }
>>
>>       size_t size() const { return UniqueFiles.size(); }
>>    +
>>    +  friend class FileManager;
>>      };
>>
>>      #endif
>>    @@ -559,6 +563,15 @@
>>       return ::stat(FilePath.c_str(), &StatBuf) != 0;
>>      }
>>
>>    +void FileManager::InvalidateCache(**const FileEntry* Entry) {
>>    +  if (!Entry)
>>    +    return;
>>
>
Why do you support a null entry being passed in at all? What's the use case
for that?


>    +
>>    +  SeenFileEntries.erase(Entry->**getName());
>>    +  UniqueRealFiles.UniqueFiles.**erase(*Entry);
>>
>
Mutating these data structures in place seems like a really unfortunate
strategy. It precludes lots of design choices down the road such as making
data structures immutable, or otherwise optimizing for the expect case of
the cache remaining valid. It also punches a hole through any hope of
sharing this cache across compilations.


I don't understand the motivation to drop *only* a single file really. For
the purposes of Cling, why not just add a non-caching made to the entire
setup? In general, I would prefer to drop the *entire* cache than bother
with fine-grained cache eviction as it doesn't seem clear that the
complexity and implementation constraints fine-grained invalidation impose
are justified by the gains.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120627/6f78e0e7/attachment.html>


More information about the cfe-commits mailing list