bugGNU Octave - Bugs: bug #53650, build: silence clang compiler...

 
 

bug #53650: build: silence clang compiler warning "lambda capture 'zero' is not required to be captured for this use"

Submitter:  Mike Miller <mtmiller>
Submitted:  Sat 14 Apr 2018 08:13:46 AM UTC
   
 
Category:  Configuration and Build System Severity:  2 - Minor
Priority:  3 - Low Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  mtmiller
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 13 Jul 2018 01:16:49 AM UTC, comment #15: 
Mike Miller <mtmiller>
Group Member
Wed 11 Jul 2018 06:34:10 PM UTC, comment #14: 

@Mike: I don't think we need to keep this bug report open any longer.  Go ahead and change the code to use constexpr.

Rik <rik5>
Group administrator
Mon 02 Jul 2018 10:13:55 PM UTC, comment #13: 

I'm fine with your suggestion to use constexpr in comment #8.

Rik <rik5>
Group administrator
Mon 02 Jul 2018 09:47:51 PM UTC, comment #12: 

Re-titling to reflect the one remaining warning, I have opened a separate bug (bug #54237) that covers the warning message about the unused 'abs' function overload.

Mike Miller <mtmiller>
Group Member
Tue 17 Apr 2018 06:41:21 PM UTC, comment #11: 

I pushed a fix for the unused parameter warning on the stable branch

https://hg.savannah.gnu.org/hgweb/octave/rev/125f73286ecf

This is my last change for this bug on the stable branch.

We can tackle the lambda issue on the default branch, and the unused function warning is in a "FIXME" section in pr-output.cc, maybe that can be reworked more fully on the default branch also.

Mike Miller <mtmiller>
Group Member
Sun 15 Apr 2018 10:42:05 PM UTC, comment #10: 

So it seems appropriate to resolve the others on the stable branch and the lambda expression one on the development branch.

Rik <rik5>
Group administrator
Sun 15 Apr 2018 07:11:13 PM UTC, comment #9: 

Ok, playing a bit more, this rewrite


    static constexpr T zero = T ();

    return std::count_if (d, d + nel,
                          [] (T elem) { return elem != zero; });


works for me with both GCC 7 and clang 6 and produces no warnings with both compilers.

I do not feel safe making this change on stable at this point without some testing under older compilers (GCC 4.8, clang 3.8).

Mike Miller <mtmiller>
Group Member
Sun 15 Apr 2018 07:01:54 PM UTC, comment #8: 

I don't know enough about lambdas to speculate about whether this warning is valid or not.

All I can say is that this warning message comes in two forms, "lambda capture 'foo' is not used", and "lambda capture 'foo' is not required to be captured for this use", which is the one we are getting here.

I looked at cppreference, and they say that a value can be used without a capture when it is constexpr. So I tried the following


    constexpr T zero = T ();

    return std::count_if (d, d + nel,
                          [] (T elem) { return elem != zero; });


This compiles just fine with GCC 7, and seems correct if I'm understanding the spec, but clang 6 throws an error.

So far the only changes that work for me with no warnings and no errors for both compilers are to make 'zero' a non-const variable, or to remove 'zero' entirely and use the 'T()' constructor inline in the lambda expression.

Mike Miller <mtmiller>
Group Member
Sun 15 Apr 2018 02:54:27 PM UTC, comment #7: 

I'm pretty sure at this point that the clang warning is not to be trusted.  How would zero make it in to the lambda function other than by capture or parameter?  According to the documentation for std::count_if (http://www.cplusplus.com/reference/algorithm/count_if/) the predicate function test has to be a unary function.  That means zero can't be passed in via a parameter.  On the other hand, If I remove the capture for zero then gcc throws an error


./liboctave/array/MDiagArray2.h: In lambda function:
./liboctave/array/MDiagArray2.h:97:56: error: ‘zero’ is not captured
                           [] (T elem) { return elem != zero; });
                                                        ^
./liboctave/array/MDiagArray2.h:97:28: note: the lambda has no capture-default
                           [] (T elem) { return elem != zero; });
                            ^
./liboctave/array/MDiagArray2.h:94:13: note: ‘const T zero’ declared here
     const T zero = T ();
             ^


I suppose we could drop this entirely and go back to a for loop


  octave_idx_type nnz (void) const
  {
    const T *d = this->data ();

    const octave_idx_type nel = this->length ();

    const T zero = T ();

    octave_idx_type nnz = 0;
    for (octave_idx_type i = 0; i < nel; i++)
      {
        if (*d != zero)
          nnz++;
        d++;
      }

   return nnz;
  }



Rik <rik5>
Group administrator
Sun 15 Apr 2018 05:42:39 AM UTC, comment #6: 

Changing the lambda to '[&zero] (T elem) { return elem != zero; }' results in the same warning message.

If I remove the const qualifier from the declaration of zero, making it a normal non-const variable, then the warning message goes away.

Mike Miller <mtmiller>
Group Member
Sun 15 Apr 2018 04:59:15 AM UTC, comment #5: 

What I was trying to do was call the empty constructor of T just once and therefore I did it outside the call to std::count_if.  I also used const just to emphasize that 'zero' is really a constant value that needs to be compared to each elem.

For std::count_if, the lamda function should be inlined, so no function calling overhead, but it will still be called nel times.  I don't want to call the constructor of T nel times.  Now, since this is a matrix class it is likely that T is a POD type in which case the constructor is trivial.  But maybe T is a complex number for which there is still some overhead in calling the constructor.

If anything, it might be better to define the capture as capture by reference with


[&zero]


Currently it is passing zero via a copy mechanism which might be calling the copy constructor of T which seems like unnecessary work.

Rik <rik5>
Group administrator
Sun 15 Apr 2018 04:49:14 AM UTC, comment #4: 

I disagree with clang about the unused-lambda-capture.  Either 'zero' has to to be known through the lamda capture mechanism, or it needs to be passed in as a parameter to the lamda function.

Rik <rik5>
Group administrator
Sun 15 Apr 2018 01:37:44 AM UTC, comment #3: 

Rik - the one I don't understand fully is the unused lambda capture warning, which is about some code that you wrote last year in 206a7eb1561b.

Is there some performance reason for what you wrote


    const T zero = T ();

    return std::count_if (d, d + nel,
                          [zero] (T elem) { return elem != zero; });


instead of this


    return std::count_if (d, d + nel,
                          [] (T elem) { return elem != T (); });


I made that change as in the attached diff, but I'm not sure how to test it. The octave_base_diag class doesn't use the DiagMatrix nnz specialization. No functional changes, test suite runs fine.

(file #43930)

Mike Miller <mtmiller>
Group Member
Sat 14 Apr 2018 09:22:36 PM UTC, comment #2: 

Thanks. I think the others might be easy to fix too, I just didn't have time to look at the details yet, I will try to get back to this in the next couple days.

Mike Miller <mtmiller>
Group Member
Sat 14 Apr 2018 12:53:49 PM UTC, comment #1: 

What is the suggested course forward?  Should we definitely fix these before 4.4 because we won't be able to change them after?  Or, since they are minor, do we fix them in 5.0 in 8 months?

I'm okay either way, and I had already noticed that m_force_gui was unused.  I removed the m_force_gui data member from class cmdline_options here (http://hg.savannah.gnu.org/hgweb/octave/rev/b20d0736f322).

Rik <rik5>
Group administrator
Sat 14 Apr 2018 08:13:46 AM UTC, original submission:  

The following compiler warnings are issued by clang 6.0 on Debian when building the current 4.3.90 stable branch head (f9fe8cf64e09).

Three of these warnings occur in declarations in public header files that are part of the Octave API, so they must not be fixed on stable after 4.4 is released.

  • -Wunused-function



../libinterp/corefcn/pr-output.cc:1722:1: warning: unused function 'abs' [-Wunused-function]
SPECIALIZE_UABS(long long)
^
../libinterp/corefcn/pr-output.cc:1713:3: note: expanded from macro 'SPECIALIZE_UABS'
  abs (unsigned T x)                            \
  ^


  • -Wunused-lambda-capture



In file included from ../liboctave/array/MArray-s.cc:43:
../liboctave/array/MDiagArray2.h:97:28: warning: lambda capture 'zero' is not required to be captured for this use [-Wunused-lambda-capture]
                          [zero] (T elem) { return elem != zero; });
                           ^
../liboctave/array/MArray-s.cc:46:27: note: in instantiation of member function 'MDiagArray2<short>::nnz' requested here
template class OCTAVE_API MDiagArray2<short>;
                          ^


  • -Wunused-parameter



In file included from ../libinterp/octave-value/ov-base.cc:59:
../libinterp/corefcn/pr-output.h:61:37: warning: unused parameter 'c' [-Wunused-parameter]
make_format (const std::complex<T>& c)
                                    ^


  • -Wunused-private-field



In file included from ../libinterp/octave.cc:44:
../libinterp/octave.h:136:10: warning: private field 'm_force_gui' is not used [-Wunused-private-field]
    bool m_force_gui = false;
         ^


There are more warnings from the use of the '-pthread' option when linking, but this is more of a build-system detail that we probably don't want to try to intervene into.

Other than these warnings, no build problems and Octave passes most tests.

Mike Miller <mtmiller>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #43930:  warnings.diff added by mtmiller (851B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mtmiller (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-13 mtmiller StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-07-11 rik5 StatusPostponed In Progress
        Assigned toNone mtmiller
    2018-07-02 mtmiller Summarybuild: silence clang compiler warnings build: silence clang compiler warning "lambda capture 'zero' is not required to be captured for this use"
    2018-04-17 mtmiller Summarybuild: clang-6.0 compiler warnings build: silence clang compiler warnings
    2018-04-17 mtmiller Priority5 - Normal 3 - Low
        StatusIn Progress Postponed
        Release4.3.90 dev
    2018-04-15 rik5 StatusNeed Info In Progress
    2018-04-15 mtmiller Attached File- Added warnings.diff, #43930
    2018-04-14 rik5 StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code