[llvm-commits] [llvm] r140712 - in /llvm/trunk/tools: Makefile llvm-cov/ llvm-cov/CMakeLists.txt llvm-cov/GCOVReader.cpp llvm-cov/GCOVReader.h llvm-cov/Makefile llvm-cov/llvm-cov.cpp

[Devang Patel]

Hi Nick,

On Sep 28, 2011, at 6:01 PM, Nick Lewycky wrote:

> On 28 September 2011 11:50, Devang Patel <dpatel at apple.com> wrote:
>> Author: dpatel
>> Date: Wed Sep 28 13:50:00 2011
>> New Revision: 140712
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=140712&view=rev
>> Log:
>> Introduce llvm-cov.
> 
> Cool!!
> 
> At a high-level, is there any interest in creating a GCOV data storage
> for reading/manipulating/writing? I didn't think anyone would ever
> need to write llvm-cov, so I decided to make GCOVProfiling write-only,
> but you have types with the same name here.

Types could be refactored into a common header.

>> +       Functions.push_back(GFun);
>> +    }
>> +    else {
>> +      delete GFun;
>> +      break;
>> +    }
>> +    ++i;
>> +  }
> 
> This is a really confusing loop.

I simplified the loop.

>> +/// read operations.
>> +class GCOVBuffer {
>> +public:
>> +  GCOVBuffer(MemoryBuffer *B) : Buffer(B), Cursor(0) {}
>> +
>> +  /// readGCOVFormat - Read GCOV signature at the beginning of buffer.
>> +  enum GCOVFormat readGCOVFormat() {
>> +    StringRef Magic = Buffer->getBuffer().slice(0, 12);
>> +    Cursor = 12;
>> +    if (Magic == "oncg*404MVLL")
>> +      return GCNO_404;
>> +    else if (Magic == "oncg*204MVLL")
>> +      return GCNO_402;
>> +    else if (Magic == "adcg*404MVLL")
>> +      return GCDA_404;
>> +    else if (Magic == "adcg*204MVLL")
>> +      return GCDA_402;
> 
> Do we really want to check the "LLVM" part? With this, we can't read
> GCC-produced .gcov files, and that really limits how useful this code
> can ever be. In particular, we can't test it against gcc-generated
> gcov files.

I don't care. Feel free to enable reading of gcc produced .gcov file. Note, I only reversed engineered CGOVProfiling.cpp, so changes may be required to read gcc produced .gcov files. I don't know.

>> +class GCOVFile {
>> +public:
>> +  GCOVFile() : Format(InvalidGCOV) {}
>> +  ~GCOVFile();
>> +  bool read(GCOVBuffer &Buffer);
>> +  void dump();
>> +  void collectLineCounts(FileInfo &FI);
>> +private:
>> +  enum GCOVFormat Format;
> 
> ...but, it stores a single format even though read() is supposed to
> take either/both the GCNO and GCDA. As far as I can tell, Format can
> be hoisted into being a local variable inside read(), because that's
> the only method that uses it.

Yup, done.

I made other cosmetic changes. If I missed anything feel free to fix.
Thanks for the review!
-
Devang