[PATCH] Value profiling - patchset 2 - merge intended
Betul Buyukkurt
betulb at codeaurora.org
Sat Jun 20 10:42:57 PDT 2015
> David Blaikie <dblaikie at gmail.com> writes:
>> On Jun 19, 2015 6:54 PM, "Justin Bogner" <mail at justinbogner.com> wrote:
>>>
>>> Justin Bogner <mail at justinbogner.com> writes:
>>> > I've gone ahead and committed a clang-formatted version of this in
>>> > r240206.
>>>
>>> I had to revert this, as some bots were failing:
>>>
>>> http://lab.llvm.org:8011/builders/sanitizer-windows/builds/5640
>>> http://lab.llvm.org:8011/builders/lldb-x86-win7-msvc/builds/5757
>>> http://lab.llvm.org:8011/builders/lldb-x86-windows-msvc/builds/7067
>>>
>>> On looking at the error it looks like you're move'ing from a vector
>>> that
>>> you've taken by reference. That seems pretty wrong. I guess you wanted
>>> an rvalue reference there?
Yes, I wanted to use rvalue reference there because of the constant
complexity. One of the comments in the non-split version of the CL's were
the move from ArrayRef to std::vector causing copying of data around which
I agree and was concerned about from the beginning. Using rvalue
references fits the usage scenario here. I'll continue to use them to not
cause copying of vectors around in the IndexedReader at least.
>>
>> It's correct(shouldn't error) to move from a non-const lvalue reference,
>> but
>> yes, probably not good style/intended. Instead pass by value.
>
> Right. Looking again, I was misled due to MSVC's hard-to-read error
> messages. The compilation error here was actually "unknown identifier:
> uint64_t" - stdint needs to be included.
>
> Betul, please fix both of these issues. Thanks.
>
Thanks for taking the time to merge and waiting for the buildbots. In my
local builds and testing, I had not come across w/ this on linux.
More information about the llvm-commits
mailing list