[llvm-dev] Redundant Twine->StringRef->Twine conversions in llvm::sys::fs::make_absolute?

[<Alexander G. Riccio> via llvm-dev]

>
> Are you talking about the local (static) function make_absolute, not the
> llvm::sys::fs::make_absolute ?
>

I'm referring to the static function... the funny thing is that the static
function is actually defined in the llvm::sys::fs namespace.

Anyhow, StringRef p() is not constructed from the Twine current_directory
> but from the SmallVector &path which is the relative path to being made
> absolute,
>

Oh, duh. I'm not sure how I missed that while typing my email. That kinda
moots the whole point of writing a patch. Nevermind.

Sincerely,
Alexander Riccio
--
"Change the world or go home."
about.me/ariccio

<http://about.me/ariccio>
If left to my own devices, I will build more.
⁂

On Mon, Apr 18, 2016 at 6:41 PM, Yaron Keren <yaron.keren at gmail.com> wrote:

> Are you talking about the local (static) function make_absolute, not the
> llvm::sys::fs::make_absolute ?
>
> Anyhow, StringRef p() is not constructed from the Twine current_directory
> but from the SmallVector &path which is the relative path to being made
> absolute, .This StringRef(SmallVector) construction should be zero-cost.
>
> The Twine current_directory is constructed only later,
> if use_current_directory. It may still be possible to optimize this code
> path but first check how much the use_current_directory=true is code path
> used vs the use_current_directory=false code path.
>
>
>
> 2016-04-19 0:55 GMT+03:00 <Alexander G. Riccio> via llvm-dev <
> llvm-dev at lists.llvm.org>:
>
>> llvm::sys::fs::make_absolute converts its first parameter (const Twine
>> &current_directory) to StringRef p(path.data(), path.size()), and then
>> passes that StringRef to several functions (path::has_root_directory,
>> path::has_root_name, and path::append) that accept Twines as parameters.
>> Since llvm::StringRef can implicitly convert to an llvm::Twine, p
>> converts to a bunch of Twine temporaries.
>>
>> In a few places, namely path::has_root_directory & path::has_root_name,
>> that temporary Twine is again converted to a StringRef:
>>
>> bool has_root_directory(const Twine &path) {
>>   SmallString<128> path_storage;
>>   StringRef p = path.toStringRef(path_storage);
>>
>>   return !root_directory(p).empty();
>> }
>>
>> Is there some reason for this? If not, I'll write a patch to delay the StringRef
>> p(path.data(), path.size()) construction until it's actually needed
>> (calls to path::root_name(p) & path::relative_path(p)).
>>
>> Sincerely,
>> Alexander Riccio
>> --
>> "Change the world or go home."
>> about.me/ariccio
>>
>> <http://about.me/ariccio>
>> If left to my own devices, I will build more.
>> ⁂
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/131009f5/attachment-0001.html>