[llvm] r202225 - Use StringRef in raw_fd_ostream constructor

[Ben Langmuir]

On Feb 26, 2014, at 4:57 PM, Alp Toker <alp at nuanti.com> wrote:

> 
> On 26/02/2014 17:16, Ben Langmuir wrote:
>> On Feb 26, 2014, at 7:16 AM, Ben Langmuir <blangmuir at apple.com> wrote:
>> 
>>> On Feb 25, 2014, at 9:49 PM, Alp Toker <alp at nuanti.com> wrote:
>>> 
>>>> On 26/02/2014 03:21, Ben Langmuir wrote:
>>>>> Author: benlangmuir
>>>>> Date: Tue Feb 25 21:21:00 2014
>>>>> New Revision: 202225
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=202225&view=rev
>>>>> Log:
>>>>> Use StringRef in raw_fd_ostream constructor
>>>>> 
>>>>> Modified:
>>>>>    llvm/trunk/include/llvm/Support/raw_ostream.h
>>>>>    llvm/trunk/lib/Support/raw_ostream.cpp
>>>>> 
>>>>> Modified: llvm/trunk/include/llvm/Support/raw_ostream.h
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/raw_ostream.h?rev=202225&r1=202224&r2=202225&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm/Support/raw_ostream.h (original)
>>>>> +++ llvm/trunk/include/llvm/Support/raw_ostream.h Tue Feb 25 21:21:00 2014
>>>>> @@ -346,7 +346,7 @@ public:
>>>>>   /// itself to own the file descriptor. In particular, it will close the
>>>>>   /// file descriptor when it is done (this is necessary to detect
>>>>>   /// output errors).
>>>>> -  raw_fd_ostream(const char *Filename, std::string &ErrorInfo,
>>>>> +  raw_fd_ostream(StringRef Filename, std::string &ErrorInfo,
>>>>>                  sys::fs::OpenFlags Flags);
>>>> Filenames are a special case in LLVM that get passed around as an opaque const char* often without ever needing having their length measured (like the clang driver code I'm looking at right now).
>>>> 
>>>> Your change will incur a strlen() upon every ctor call: not a great problem, but also somewhat redundant -- was there a need that triggered this change?
>>> I intend to make FileEntry in Clang own the storage for its file name,
> 
> Anecdotally, I believe FileEntry used to have storage with an std::string back in the days before SVN history. There's also an old proposal in NOTES.txt to change it to a sys::Path (which doesn't exist any more). These days it's a POD structure that gets passed around a lot so I'd think twice about such a change if it's avoidable.

Only references to FileEntry are passed around.  There is exactly one place in Clang that copies a FileEntry (and as a point of interest, it is safe to use a move constructor instead).  I think having FileEntry own the storage is going to be fine.

> 
> 
>>>  which seemed like a good opportunity to change the FileEntry getName to return a StringRef, particularly because it is currently being used/abused to also check if the FileEntry is initialized.  This was one of the pieces that uses that name, so I thought I’d avoid a string copy before passing it in.
>>> 
>>> What if I switch to raw_fd_ostream(const Twine &Filename, …)?  Then if it is null-terminated we don’t need the strlen call.
>> Actually, given what you just told me, is changing FIleEntry to use StringRef even a good idea? I could potentially just use c_str() to get the pointer now that I have identified all the places that were checking for a null result.
> 
> Although there's no strict convention, const char* has benefits over StringRef for file paths:
> 
> * Discourages non-portable path manipulation/comparison. Wherever we've passed around paths as StringRefs sooner or later someone goes ahead and tries if (a == b) ... which passes tests but fails in real environments.

As David Blaikie points out, I see several uses of strcmp, some of which look totally safe, others I’m not sure about.  I’ll also point out that the sys::path APIs generally use StringRef.

> 
> * Ensures the presence of a null terminator. Low-level filesystem ops require a null-terminated C string so if we pass around StringRefs there's a risk we'll have to incur a copy just to null-terminate the string. Not requiring strlen() is a corollary to that.

Okay, I buy this.  There are some places that construct StringRefs from filenames, so we are paying this cost elsewhere, but I have no data to say that I would be improving the situation.

> 
> So If you can get away with c_str(), it's best to keep the const char* status quo IMO. If that isn't possible, a Twine is a second best (indeed the underlying LLVM abstractions work with a Twine).

 I’ll revert this patch and use c_str.

> If however it turns out you actually do need storage for FileEntry, we should review the changes together in context because that has wider impact.

I do need storage, but I’ll keep that hidden in the FileEntry for now.

Thanks,

Ben

> 
> Alp.
> 
> 
> 
>> 
>> Ben
>> 
>>> Ben
>>> 
>>>> Alp.
>>>> 
>>>>>     /// raw_fd_ostream ctor - FD is the file descriptor that this writes to.  If
>>>>> 
>>>>> Modified: llvm/trunk/lib/Support/raw_ostream.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/raw_ostream.cpp?rev=202225&r1=202224&r2=202225&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Support/raw_ostream.cpp (original)
>>>>> +++ llvm/trunk/lib/Support/raw_ostream.cpp Tue Feb 25 21:21:00 2014
>>>>> @@ -430,16 +430,15 @@ void format_object_base::home() {
>>>>> /// occurs, information about the error is put into ErrorInfo, and the
>>>>> /// stream should be immediately destroyed; the string will be empty
>>>>> /// if no error occurred.
>>>>> -raw_fd_ostream::raw_fd_ostream(const char *Filename, std::string &ErrorInfo,
>>>>> +raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::string &ErrorInfo,
>>>>>                                sys::fs::OpenFlags Flags)
>>>>>     : Error(false), UseAtomicWrites(false), pos(0) {
>>>>> -  assert(Filename != 0 && "Filename is null");
>>>>>   ErrorInfo.clear();
>>>>>     // Handle "-" as stdout. Note that when we do this, we consider ourself
>>>>>   // the owner of stdout. This means that we can do things like close the
>>>>>   // file descriptor when we're done and set the "binary" flag globally.
>>>>> -  if (Filename[0] == '-' && Filename[1] == 0) {
>>>>> +  if (Filename == "-") {
>>>>>     FD = STDOUT_FILENO;
>>>>>     // If user requested binary then put stdout into binary mode if
>>>>>     // possible.
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> -- 
>>>> http://www.nuanti.com
>>>> the browser experts
> 
> -- 
> http://www.nuanti.com
> the browser experts