[PATCH] D92531: Basic: Support named pipes natively in SourceManager

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith marked an inline comment as done.
dexonsmith added a comment.

Thanks for the review!



================
Comment at: clang/lib/Basic/SourceManager.cpp:167
+  // (which may have come from a stat cache).
+  if (ContentsEntry->isNamedPipe()) {
+    // Check the buffer size directly if this is a named pipe.
----------------
arphaman wrote:
> Would it make sense to do the `diagnoseFileTooLarge` check for both files and named pipes here unconditionally instead of having two separate checks? It appears that it shouldn't really matter if we check the ContentsEntry or the Buffer size for regular files, as their sizes are expected to match anyway.
To explain, the reason I split it was to be able to return faster (don't bother loading the buffer) if it's going to be the wrong size... but an `mmap` is cheap and the error path isn't worth micro-optimizing.

I agree it's cleaner to just move it. I'll update to do that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92531/new/

https://reviews.llvm.org/D92531