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

 
 

bug #53650: build: silence clang compiler warnings

Submitted by:  Mike Miller <mtmiller>
Submitted on:  Sat 14 Apr 2018 08:13:46 AM UTC  
 
Category: Configuration and Build SystemSeverity: 2 - Minor
Priority: 3 - LowItem Group: Unexpected Error
Status: PostponedAssigned to: None
Originator Name: Open/Closed: Open
Release: devOperating 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 Jump to the original submission)

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

 

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

Attach File(s):
   
   
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
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by mtmiller (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 17 Apr 2018 06:41:44 PM UTCmtmillerSummarybuild: clang-6.0 compiler warnings=>build: silence clang compiler warnings
    Tue 17 Apr 2018 06:41:21 PM UTCmtmillerPriority5 - Normal=>3 - Low
      StatusIn Progress=>Postponed
      Release4.3.90=>dev
    Sun 15 Apr 2018 10:42:05 PM UTCrik5StatusNeed Info=>In Progress
    Sun 15 Apr 2018 01:37:44 AM UTCmtmillerAttached File-=>Added warnings.diff, #43930
    Sat 14 Apr 2018 12:53:49 PM UTCrik5StatusNone=>Need Info

    Back to the top


    Powered by Savane 3.1-cleanup1