On 11/07/2017 03:49 PM, Nathan Sidwell wrote:
> On 11/07/2017 05:53 AM, Martin Liška wrote:
>> Hello.
>>
>> This is slightly updated version from the previous. Various small issues were fixed
>> and I update documentation in order to reflect the changes.
> 
>> +  gcov_write_unsigned (DECL_ARTIFICIAL (current_function_decl));
>>    gcov_write_filename (xloc.file);
>>    gcov_write_unsigned (xloc.line);
>> +  gcov_write_unsigned (expand_location (cfun->function_end_locus).line);
>>    gcov_write_length (offset);
> 
> this is presuming the end line is in the same file as the start line.  A reasonable assumption, but users can have exciting ideas!  What is the failure mode if the function straddles a file boundary?

Hi.

I decided to fix that with change that set line_end = line_start if line_end < line_start.
That survives reasonably well with cases like this:

$         -:    3:template<class T>
        -:    4:class Foo
        -:    5:{
        -:    6:  public:
        1:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
Foo<char>::Foo():
        0:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
Foo<int>::Foo():
        1:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
Foo<char>::Foo(int):
        0:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
Foo<int>::Foo(int):
        0:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
Foo<char>::xxx():
        0:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
Foo<int>::xxx():
        0:    7:  Foo(): b (1000) {}          Foo(int _b): b (_b) {}   static void xxx() {}
------------------
        2:    8:  void inc () {
------------------
Foo<char>::inc():
        0:    8:  void inc () {
------------------
Foo<int>::inc():
        2:    8:  void inc () {
------------------
        2:    9:    b++;
[another gcov file]
        2:    3:b++;
        2:    4:b++;
        2:    5:b += 1;
        2:    6:}

> 
> +  /* True when multiple functions start at a line in a source file.  */
> +  unsigned is_group : 1;
> 
> Is this true for the non-template instantiation case of:
> 
> void a () {} void b () {}
> 
> What happens there?  (perhaps we cannot tell the difference?)
> 
> +  inline bool operator() (const function_info *lhs,
> +              const function_info *rhs)
> +    {
> +      return lhs->end_line < rhs->start_line;

There's typo as s/end_line/start_line.

> +    }
> 
> This isn't stable if they start on the same line.  Will output order depend on the vaguaries of the sorting algorithm?
> 
>> +      vector<line_info> &lines = (*it2)->lines;
>> +      /* Print all lines covered by the function.  */
>> +      for (unsigned i = 0; i < lines.size (); i++)

So fixed by introduction of line column that is used for sorting as well.

Now we have:

        -:    2:class Foo
        -:    3:{
        -:    4:  public:
        -:    5:  int b;
        1:    6:  Foo(): b (1000) {}
       2*:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
Foo<int>::inc():
        1:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
test():
    #####:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
test2():
    #####:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
test3():
    #####:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
test4():
        1:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
test5():
    #####:    7:  void inc () { if (b) b++; } };     static void test() {}   static void test2() {} static void test3() {} static void test4() {} static void test5() {}
------------------
        -:    8:
        1:    9:int main()
        -:   10:{
        1:   11:  Foo<int> xx;
        1:   12:  xx.inc();
        -:   13:
        1:   14:  test4();
        -:   15:
        1:   16:  return 0;
        -:   17:}

Which hopefully also work fine.

> 
> comment seems to apply to the whole block, not just the for loop? The lines init is merely local cache?  (this stood out because of lack of blank line before the comment, btw)

Removed it :)

> 
> Otherwise looks good, and a nice improvement.  Please investigate the corner cases mentioned above though.

Thank you.
Btw. there are people in Mozilla that have been utilizing gcov significantly:
https://marco-c.github.io/2017/07/28/code-coverage-architecture.html

They would definitely benefit from the functionality because the code base is full of heavy C++ code.

May I understand the reply as ACK?
Martin

> 
> nathan