bruno added inline comments.

================
Comment at: lib/Driver/ToolChains.cpp:3915
 
-  if (D.getVFS().exists("/etc/SuSE-release"))
-    return OpenSUSE;
+  File = llvm::MemoryBuffer::getFile("/etc/SuSE-release");
+  if (File)
----------------
mgorny wrote:
> bruno wrote:
> > You should keep using the VFS to obtain the file.  You probably also want 
> > to add a comment here to describe what you mentioned in the patch Summary.
> Hmm, this method is consistent with all the other distributions in the 
> method. It seems that `getVFS()` is only used for `exists()` checks there. 
> Are you sure I should change this, without touching the other reads first?
Right, there seems to be some inconsistent usage here.  Ideally we should go 
through the VFS, but given the precedence it's understandable if you don't make 
it in this patch.

AFAIU, this drops support for SLES10 and I guess that this would also be true 
for SLES9? It seems that the checking could be improved a bit by checking the 
digits (as in the distros above)? Also add a comment explaining why.


https://reviews.llvm.org/D24954



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to