<div style="font-family: arial, helvetica, sans-serif"><font size="2">This thread seems fine to discuss it -- I didn't want to have missed an existing discussion.<div><br></div><div>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.</div>
<div><br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Wed, Jun 27, 2012 at 2:17 AM, Axel Naumann <<a href="mailto:Axel.Naumann@cern.ch" target="_blank" class="cremed">Axel.Naumann@cern.ch</a><br></div><div><div class="h5">
<mailto:<a href="mailto:Axel.Naumann@cern.ch" target="_blank" class="cremed">Axel.Naumann@cern.ch</a>>> wrote:<br>
<br>
Author: axel<br>
Date: Wed Jun 27 04:17:42 2012<br>
New Revision: 159256<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=159256&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<u></u>project?rev=159256&view=rev</a><br>
Log:<br>
>From Vassil Vassilev:<br>
add interface for removing a FileEntry from the cache.<br>
Forces a re-read the contents from disk, e.g. because a tool (like<br>
cling) wants to pick up a modified file.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/<u></u>FileManager.h<br>
cfe/trunk/lib/Basic/<u></u>FileManager.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/<u></u>FileManager.h<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=159256&r1=159255&r2=159256&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/include/<u></u>clang/Basic/FileManager.h?rev=<u></u>159256&r1=159255&r2=159256&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/include/clang/Basic/<u></u>FileManager.h (original)<br>
+++ cfe/trunk/include/clang/Basic/<u></u>FileManager.h Wed Jun 27 04:17:42 2012<br>
@@ -222,6 +222,9 @@<br>
/// FileManager's FileSystemOptions.<br>
bool getNoncachedStatValue(<u></u>StringRef Path, struct stat &StatBuf);<br>
<br>
+ /// \brief Remove the real file Entry from the cache.<br>
+ void InvalidateCache(const FileEntry* Entry);<br>
+<br></div></div></blockquote></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<div class="h5">
/// \brief If path is not absolute and FileSystemOptions set the<br>
working<br>
/// directory, the path is modified to be relative to the given<br>
/// working directory.<br>
<br>
Modified: cfe/trunk/lib/Basic/<u></u>FileManager.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=159256&r1=159255&r2=159256&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/lib/Basic/<u></u>FileManager.cpp?rev=159256&r1=<u></u>159255&r2=159256&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- cfe/trunk/lib/Basic/<u></u>FileManager.cpp (original)<br>
+++ cfe/trunk/lib/Basic/<u></u>FileManager.cpp Wed Jun 27 04:17:42 2012<br>
@@ -111,6 +111,8 @@<br>
}<br>
<br>
size_t size() const { return UniqueFiles.size(); }<br>
+<br>
+ friend class FileManager;<br></div></div></blockquote></blockquote><div><br></div><div>Adding these friends seems extremely unfortunate. Why are they required?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
};<br>
<br>
//===-------------------------<u></u>------------------------------<u></u>---------------===//<br>
@@ -152,6 +154,8 @@<br>
}<br>
<br>
size_t size() const { return UniqueFiles.size(); }<br>
+<br>
+ friend class FileManager;<br>
};<br>
<br>
#endif<br>
@@ -559,6 +563,15 @@<br>
return ::stat(FilePath.c_str(), &StatBuf) != 0;<br>
}<br>
<br>
+void FileManager::InvalidateCache(<u></u>const FileEntry* Entry) {<br>
+ if (!Entry)<br>
+ return;<br></div></div></blockquote></blockquote><div><br></div><div>Why do you support a null entry being passed in at all? What's the use case for that?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
+<br>
+ SeenFileEntries.erase(Entry-><u></u>getName());<br>
+ UniqueRealFiles.UniqueFiles.<u></u>erase(*Entry);</div></div></blockquote></blockquote><div><br></div><div>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.</div>
<div><br></div><div><br></div><div>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.</div>
</div></div></font></div>