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"

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

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 13 Jul 2018 01:16:49 AM UTC, comment #15:

Changed on default

https://hg.savannah.gnu.org/hgweb/octave/rev/8090a118eb03

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
Sun 15 Apr 2018 07:11:13 PM UTC, comment #9:

Ok, playing a bit more, this rewrite

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>
Project AdministratorIn charge of this item.
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

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>
Project AdministratorIn charge of this item.
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

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

Rik <rik5>
Project 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>
Project AdministratorIn charge of this item.
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

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>
Project 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>
Project 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

instead of this

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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project 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
  • -Wunused-lambda-capture
  • -Wunused-parameter
  • -Wunused-private-field

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>
Project AdministratorIn charge of this item.

 

(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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project 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.3