Menu

#5649 flower: Add boolean return value to 'Rational' class.

Fixed
Maintainability
2020-01-09
2019-12-29
No

This removes many clang warnings of the type

implicit conversion turns floating-point number into integer:
'double' to 'bool' [-Wfloat-conversion]

https://codereview.appspot.com/581400043

1 Attachments

Related

Issues: #5649

Discussion

1 2 > >> (Page 1 of 2)
  • Anonymous

    Anonymous - 2019-12-30

    Passes make, make check and a full make doc.

    Reg test diffs attached

     
  • Werner LEMBERG

    Werner LEMBERG - 2019-12-30

    Thanks for the diffs. It's surprising to me that there are any diffs at all. I don't see any problematic differences, though.

     
    • David Kastrup

      David Kastrup - 2019-12-30

      "Werner LEMBERG" wlemb@users.sourceforge.net writes:

      Thanks for the diffs. It's surprising to me that there are any diffs
      at all. I don't see any problematic differences, though.


      ** [issues:#5649] flower: Add boolean return value to 'Rational' class.**

      Status: Started
      Created: Sun Dec 29, 2019 10:08 AM UTC by Werner LEMBERG
      Last Updated: Mon Dec 30, 2019 05:06 AM UTC
      Owner: Werner LEMBERG
      Attachments:

      -
      test-results.zip
      (497.6 kB; application/zip)

      This removes many clang warnings of the type

      implicit conversion turns floating-point number into integer:
      'double' to 'bool' [-Wfloat-conversion]

      https://codereview.appspot.com/581400043

      I think it would still be good to know how they come about. Maybe we
      get zero values by having the numerator rather than the sign zero? If
      so, it would be important to figure out where.

      --
      David Kastrup

       

      Related

      Issues: #5649

      • Werner LEMBERG

        Werner LEMBERG - 2019-12-31

        I think it would still be good to know how they come about.
        Maybe we get zero values by having the numerator rather
        than the sign zero? If so, it would be important to figure out
        where.

        Attached is a log file of

        make EXTRA_CXXFLAGS="-W -Wall -Wconversion -Wno-sign-conversion"
        

        for commit afb36349 (i.e., without my patch) that shows the warning locations.

         
  • David Kastrup

    David Kastrup - 2019-12-30

    Which clang version?

     
    • Werner LEMBERG

      Werner LEMBERG - 2019-12-31

      clang++ 9.0 on MacOS Lion.

       
  • Anonymous

    Anonymous - 2019-12-31

    Leaving on Review for now.

     
  • Anonymous

    Anonymous - 2020-01-03
    • Patch: review --> countdown
     
  • Anonymous

    Anonymous - 2020-01-03

    Moving to countdown for Jan 5th - if you want to change this (because of the reg test diffs) feel free.

     
  • Anonymous

    Anonymous - 2020-01-05
    • Patch: countdown --> push
     
  • Anonymous

    Anonymous - 2020-01-05

    This patch has been counted down to push. If you want to change this (because of the reg test diffs) feel free.

     
  • David Kastrup

    David Kastrup - 2020-01-05
    • Patch: push --> waiting
     
  • David Kastrup

    David Kastrup - 2020-01-05

    I have not yet found a good reason for the regtest changes. Let me try some more experiments. Put to waiting, but I'll not hold it up more than a few days.

     
    • Werner LEMBERG

      Werner LEMBERG - 2020-01-05

      Thanks for working on this!

       
      • David Kastrup

        David Kastrup - 2020-01-05

        I am not able to trigger a difference in one session adding debug code. I am going to try the full make check thing to see whether this can make a difference.

         
        • David Kastrup

          David Kastrup - 2020-01-05

          What's really irritating is that touching flower/include/rational.hh does not trigger any recompile. Has this always been the case or is this a recently introduced dependency problem?

           
          • Werner LEMBERG

            Werner LEMBERG - 2020-01-06

            I'm not aware of fundamental changes in this regard. Maybe Dan Eble knows more.

             
            • David Kastrup

              David Kastrup - 2020-01-06

              Sorry, sometimes the SourceForge user interface sucks. I set this to "Patch-push" again after not being able to corroborate any changes with either GCC and Clang, and the dependency problem occurs only with Clang which appears to fail at autogenerating dependencies, so you always have to do make clean before getting anything of consequence done. Which sucks, but is not a priority for our normal GCC-based process.

               
  • David Kastrup

    David Kastrup - 2020-01-05
    • Patch: waiting --> push
    • Type: Enhancement --> Maintainability
     
  • David Kastrup

    David Kastrup - 2020-01-05

    make check does not show any difference to the baseline without the patch. While this difference might depend on the compiler version, the current gcc version I use should be comparatively relevant. I have not been able to trigger any difference with either our code base or hand-crafted test programs either.

    So I lean towards considering this a fluke, possibly because of a non-up-to-date test-baseline or some compiler bug? Setting back to Patch-push.

     
  • David Kastrup

    David Kastrup - 2020-01-06
    • Patch: push --> waiting
     
  • David Kastrup

    David Kastrup - 2020-01-06

    Crikey. All of my testing used Clang. Back to waiting while I redo checks (but after the current other problem I am tracking)

     
    • David Kastrup

      David Kastrup - 2020-01-06

      Nope, still clean, no regtest differences, no behavior change when debug code checks explicitly.

       
  • David Kastrup

    David Kastrup - 2020-01-06
    • Patch: waiting --> push
     
  • David Kastrup

    David Kastrup - 2020-01-06

    make check does not show any difference to the baseline without the patch. While this difference might depend on the compiler version, the current gcc version I use should be comparatively relevant. I have not been able to trigger any difference with either our code base or hand-crafted test programs either.

    So I lean towards considering this a fluke, possibly because of a non-up-to-date test-baseline or some compiler bug? Setting back to Patch-push.

     
1 2 > >> (Page 1 of 2)