[llvm] r202225 - Use StringRef in raw_fd_ostream constructor

[David Blaikie]

On Wed, 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.
>
>
>
>>>   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.
>
> * 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.
>
> 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'm not sure why const char* is preferred when the underlying API
takes a Twine - your second point doesn't really hold up there. And
I'm not sure const char* is really going to discourage someone from
writing strcmp any more than using ==.

>
> 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.
>
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>