[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

[Nick Lewycky]

On 29 September 2011 10:13, Devang Patel <dpatel at apple.com> wrote:
> 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.

You don't sound enthusiastic :) I'm happy to leave it as two separate
pieces if there's no use-case for joining them.

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

Thanks!

>>> +/// 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!

Thanks!

Nick