[llvm] r202225 - Use StringRef in raw_fd_ostream constructor

[Alp Toker]

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).

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