Re: [llvm-commits] CVS: llvm/lib/Support/MemoryBuffer.cpp

2007-05-06 Thread Reid Spencer 
On Sun, 2007-05-06 at 02:25 -0500, Chris Lattner wrote:
 
 Changes in directory llvm/lib/Support:
 
 MemoryBuffer.cpp updated: 1.3 - 1.4
 ---
 Log message:
 
 Fix MemoryBuffer::getFile to return null if it has an error opening the
 file instead of aborting.
 
 
 ---
 Diffs of the changes:  (+20 -8)
 
  MemoryBuffer.cpp |   28 
  1 files changed, 20 insertions(+), 8 deletions(-)
 
 
 Index: llvm/lib/Support/MemoryBuffer.cpp
 diff -u llvm/lib/Support/MemoryBuffer.cpp:1.3 
 llvm/lib/Support/MemoryBuffer.cpp:1.4
 --- llvm/lib/Support/MemoryBuffer.cpp:1.3 Sun Apr 29 09:43:31 2007
 +++ llvm/lib/Support/MemoryBuffer.cpp Sun May  6 02:24:46 2007
 @@ -111,7 +111,9 @@
  class MemoryBufferMMapFile : public MemoryBuffer {
sys::MappedFile File;
  public:
 -  MemoryBufferMMapFile(const sys::Path Filename);
 +  MemoryBufferMMapFile() {}
 +  
 +  bool open(const sys::Path Filename);

Shouldn't this have a parameter to receive the error message ?


virtual const char *getBufferIdentifier() const {
  return File.path().c_str();
 @@ -121,12 +123,11 @@
  };
  }
  
 -MemoryBufferMMapFile::MemoryBufferMMapFile(const sys::Path Filename) {
 +bool MemoryBufferMMapFile::open(const sys::Path Filename) {
// FIXME: This does an extra stat syscall to figure out the size, but we
// already know the size!
bool Failure = File.open(Filename);

This needs to have an error message parameter to receive the error
message.

 -  Failure = Failure;  // Silence warning in no-asserts mode.
 -  assert(!Failure  Can't open file??);
 +  if (Failure) return true;

This is insufficient. The caller doesn't know why the
MemoryBufferMMapFile couldn't be opened.

File.map();

 @@ -147,10 +148,12 @@
  // No need to keep the file mapped any longer.
  File.unmap();
}
 +  return false;
  }
  
  MemoryBufferMMapFile::~MemoryBufferMMapFile() {
 -  File.unmap();
 +  if (File.isMapped())
 +File.unmap();
  }
  
  
 //===--===//
 @@ -161,7 +164,11 @@
  int64_t FileSize) {
sys::PathWithStatus P(FilenameStart, FnSize);
  #if 1
 -  return new MemoryBufferMMapFile(P);
 +  MemoryBufferMMapFile *M = new MemoryBufferMMapFile();
 +  if (!M-open(P))
 +return M;
 +  delete M;
 +  return 0;
  #else
// FIXME: We need an efficient and portable method to open a file and then 
 use
// 'read' to copy the bits out.  The unix implementation is below.  This is
 @@ -177,8 +184,13 @@
}

// If the file is larger than some threshold, use mmap, otherwise use 
 'read'.
 -  if (FileSize = 4096*4)
 -return new MemoryBufferMMapFile(P);
 +  if (FileSize = 4096*4) {
 +MemoryBufferMMapFile *M = new MemoryBufferMMapFile();
 +if (!M-open(P))
 +  return M;
 +delete M;
 +return 0;
 +  }

MemoryBuffer *SB = getNewUninitMemBuffer(FileSize, FilenameStart);
char *BufPtr = const_castchar*(SB-getBufferStart());
 
 
 
 ___
 llvm-commits mailing list
 llvm-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

___
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Re: [llvm-commits] CVS: llvm/lib/Support/MemoryBuffer.cpp

2007-05-06 Thread Chris Lattner 
 +  MemoryBufferMMapFile() {}
 +
 +  bool open(const sys::Path Filename);

 Shouldn't this have a parameter to receive the error message ?

No, because membuffer doesn't know why the open failed.  It would  
always return the same thing.

-Chris
___
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Re: [llvm-commits] CVS: llvm/lib/Support/MemoryBuffer.cpp

2007-05-06 Thread Reid Spencer 
On Sun, 2007-05-06 at 00:43 -0700, Chris Lattner wrote:
  +  MemoryBufferMMapFile() {}
  +
  +  bool open(const sys::Path Filename);
 
  Shouldn't this have a parameter to receive the error message ?
 
 No, because membuffer doesn't know why the open failed.  It would  
 always return the same thing.

It must be passed up from lib/System. I suggested this in my other
comments on these changes. 

It is not acceptable to force the application to print something like:

file: can't open: don't know why, we just can't

(I'm exaggerating but you get the point)

Reid.

 
 -Chris
 ___
 llvm-commits mailing list
 llvm-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

___
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Re: [llvm-commits] CVS: llvm/lib/Support/MemoryBuffer.cpp

2007-05-06 Thread Chris Lattner 

On May 6, 2007, at 12:50 AM, Reid Spencer wrote:

 On Sun, 2007-05-06 at 00:43 -0700, Chris Lattner wrote:
 +  MemoryBufferMMapFile() {}
 +
 +  bool open(const sys::Path Filename);

 Shouldn't this have a parameter to receive the error message ?

 No, because membuffer doesn't know why the open failed.  It would
 always return the same thing.

 It must be passed up from lib/System. I suggested this in my other
 comments on these changes.

 It is not acceptable to force the application to print something like:

 file: can't open: don't know why, we just can't

 (I'm exaggerating but you get the point)

Ah, I didn't realize mapped file did this.  This is great, I'll do  
this after my current set of changes.  It will simplify several clients.

-Chris

___
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits