<div dir="ltr"><div dir="ltr">Hi Sam!<div><br></div><div>I was thinking about this as well, it would be highly risky to land a massive change like this at once. I don't think it would be wise for me to try that, especially given that this was attempted before by others.</div><div><br></div><div>Right now I'm thinking about introducing a parallel API to `getFile`, which would return the FileEntryRef, and starting the migration by migrating only those clients that are needed for the initial use-case (FileManager reuse for dependency scanning). I think it would be fine if the migration happened on per-need basis, as long as new clients use the new API from the beginning. Do you think that's a good approach?</div><div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Aug 2019 at 14:12, Sam McCall <<a href="mailto:sammccall@google.com">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">I'm sympathetic to wanting to change this behaviour, as it's super subtle and weird.<div>However this is a pretty risky change because it's subtle and filename handling often doesn't have a clearly correct answer. It'd be nice if there was a way to land this patch in isolation without an API change, so it can bake for a while and be more easily reverted if need be.</div><div>One detail: IIUC, this</div><div>  >  it will return a FileEntry whose name is guaranteed to be that path</div><div>isn't quite accurate, it stat()s the the file, assigns that to InterndFileName, and the assigns that to UFE.Name. So getFile() performs some canonicalization, and callers may rely on that now (clangd does).</div></div></blockquote><div><br></div><div>Right, that's true, thanks for clarifying. Right now I don't want to change this particular aspect of this behavior. Do you know some of the scenarios when the name is different? I don't think it canonicalized symlinks, right?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 31, 2019 at 9:30 PM Alex L via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi,<div><br></div><div>As it stands right now, Clang's FileManager doesn't model file entries with multiple names correctly. Whenever the `getFile` method is called with a path it hasn't seen before, it will return a FileEntry whose name is guaranteed to be that path. However, this guarantee is lost whenever a symlink file that points to original path is opened using `getFile`, as the `name` of the FileEntry is overwritten with the symlink path. This happens because the FileManager uniques the FileEntries using inodes, which means that symlinks always get the same file entry as the original file.</div><div><br></div><div>I would like to fix this issue as it's blocking clang-scan-deps from reusing the FileManager across multiple invocations. Here's my idea:</div><div><br></div><div>- Return FileEntryRef that stores the FileEntry * and a Name.</div><div>- Don't store the Name in the FileEntry (keep the RealPathName) still.</div><div>- A comparison between FileEntryRef will compare the pointer values of FileEntries, so the code that compares the pointer values right now can still compare two FileEntryRefs with the equivalent result (important for modules).</div><div>- DenseMaps will still mostly use the FileEntry * values to preserve the existing behavior which expects one FileEntry per inode.</div><div><br></div><div>Additionally to fixing the issue described above, Harlan (CCed) and I would like to also perform the following minor cleanups and improvements:</div><div><br></div><div>- Start using error_code/ErrorOr for FileManager for improved API.</div><div>- Get rid of the MemorizeStatCalls stat cache, which isn't actually used right now by Clang (probably a bug)!</div><div>- Cache the VFS stat value in the FileEntryRef itself, so that the FileManager can actually have proper caching that works.</div><div><br></div><div>Please let me know if you have any feedback. I'm hoping to post a patch for FileEntryRef sometime in the next 1-2 weeks.</div><div><br></div><div>Thanks,</div><div>Alex</div></div></div></div></div></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>
</blockquote></div></div>