[llvm] r202225 - Use StringRef in raw_fd_ostream constructor

Alp Toker alp at nuanti.com
Wed Feb 26 18:15:21 PST 2014


On 27/02/2014 01:51, Ben Langmuir wrote:
> 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.

Yeah, these are valid observations and as David points out neither is a 
silver bullet. In the absence of a caller that explicitly needs to pass 
in non-terminated path strings I do still prefer the status quo.

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

Sounds perfect, thanks Ben.

And we should probably move the outdated FileEntry remarks in NOTES.txt 
to a PR or inline code comment if they're still relevant. Guessing you 
have a better feel for it having worked on the VFS recently if you want 
to have a shot at it..

Alp.

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

-- 
http://www.nuanti.com
the browser experts




More information about the llvm-commits mailing list