russellallen / self

Making the world safe for objects
http://selflanguage.org
713 stars 76 forks source link

Annoying warning when compiling VM #155

Closed russellallen closed 2 months ago

russellallen commented 3 months ago

This was presumably supposed to suppress warnings, but now it throws up a new warning, which is boring:

[ 41%] Building CXX object vm/CMakeFiles/Self.dir/src/any/parser/send.cpp.o
In file included from /root/build/incls/_precompiled.hh:12,
                 from /root/build/incls/_send.cpp.incl:1,
                 from /self/vm/src/any/parser/send.cpp:7:
/self/vm/src/any/runtime/util.hh: In function ‘void UsedOnlyInAssert(double)’:
/self/vm/src/any/runtime/util.hh:149:56: warning: right operand of comma operator has no effect [-Wunused-value]
  149 | inline void UsedOnlyInAssert(double x)   { (void)(x), 0; }
      |                                                        ^
krono commented 3 months ago

I think this is a default by now

kyle-github commented 3 months ago

I would probably replace that with just

{ (void)(x); return; }

A bare return on a void function is fine if not strictly needed in all cases.

On Mon, Jul 22, 2024 at 10:07 PM Tobias Pape @.***> wrote:

I think this is a default by now

— Reply to this email directly, view it on GitHub https://github.com/russellallen/self/issues/155#issuecomment-2244262325, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4LC4BDW7LLDRSOI5NRWLZNXQJVAVCNFSM6AAAAABLJFW6S6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBUGI3DEMZSGU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

krono commented 3 months ago

This is extremely old code base. I introduced the (void)(x) long time ago, but the x, 0 is there even longer. https://github.com/russellallen/self/commit/a0d9c69e29f48786bae1b9727b14e4a0f95acc47#diff-3fffbe34ae05cc7c716a611817fa2d3e440dac22dc5753f8e79bdcea89a126cbR152

I do think silencing warnings here is the OK thing to do. The C++ the codebase is written for is not the C++ current compliers thinkt it should be. but that's ok…

russellallen commented 2 months ago

I've changed as per the suggestion: https://github.com/russellallen/self/commit/4912a2fe055408fa839cccd45e8ac9598634f0c5

Now I can see all the other warnings...