[llvm-commits] [llvm] r140151 - /llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp

[Devang Patel]

On Sep 20, 2011, at 11:10 AM, Nick Lewycky wrote:

> Devang Patel wrote:
>> Author: dpatel
>> Date: Tue Sep 20 12:43:14 2011
>> New Revision: 140151
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=140151&view=rev
>> Log:
>> Eliminate unnecessary copy of FileName from GCOVLines.
>> GCOVLines is always accessed through a StringMap where the key is FileName.
>> 
>> Modified:
>>     llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp
>> 
>> Modified: llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp?rev=140151&r1=140150&r2=140151&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp Tue Sep 20 12:43:14 2011
>> @@ -167,18 +167,17 @@
>>      }
>> 
>>      uint32_t length() {
>> -      return lengthOfGCOVString(Filename) + 2 + Lines.size();
>> +      // FIXME: ??? What is the significance of 2 here ?
>> +      return 2 + Lines.size();
> 
> This no longer computes the length() though. How about changing std::string Filename into a StringRef if you're concerned about copies? Alternatively, you could name it something like "getNumLines()" and hoist out the +2 as well.

In that case I chose the alternate route and robustified GCOVLines in r140167.

> Fundamentally, GCOVLines isn't just a list of lines, it's a list of lines attached to a particular filename, so I wanted it to contain all its own data. The +2 is 1 for string length plus 1 for the '0' line id# which means "the filename is" in GCOV format.

Thanks, I updated the comments. 
-
Devang