russellallen / self

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

Warning fixes #145

Closed nbuwe closed 1 year ago

nbuwe commented 1 year ago

A few minor fixes.

I'm testing with -Wall -Wextra and a specific list of -Wno-..., but I'm a bit reluctant to pull that change to setup.cmake too b/c older gcc versions may not have some of those options.

nbuwe commented 1 year ago

For the record, the options are:

list(APPEND _flags
  -Wall
  -Wextra

  # probably ok, but needs auditing
  -Wno-delete-non-virtual-dtor

  # old use of "volatile" in lieu of attribute((noreturn))
  -Wno-ignored-qualifiers

  # TODO: just add FALLTHROUGH annotation comments
  -Wno-implicit-fallthrough

  # these are often hit or miss...
  -Wno-maybe-uninitialized

  # complains about a few cases of existing idiomatic layout
  -Wno-misleading-indentation

  # cf. -fno-delete-null-pointer-checks above
  -Wno-nonnull

  # these are a pain to fix properly...
  -Wno-sign-compare

  # this is old code from the time of less opinionated compilers
  # cf. no -fno-strict-aliasing above
  -Wno-strict-aliasing

  # vm/src/any/parser/parser.cpp uses some literal chars not in enum
  -Wno-switch

  # there are few, marked "for debugging", and they probably need to
  # be marked volatile so that those assignments are not elided and
  # the values are actually available to be inspected from the
  # debugger
  -Wno-unused-but-set-variable

  # mostly spammy for C++, needs a rainy day cleanup round
  -Wno-unused-parameter

  # needs a rainy day cleanup round
  -Wno-unused-variable
)
russellallen commented 1 year ago

This is all great.
I would say that as an old project with minimal available resources, Self's problem has always been keeping up with platform and compiler changes, not in leaving old systems behind. Ideally I'd love it if the vm compiled without warnings now, so that we're cleaner for future (maybe fussier) compilers.

Anyway, after merging all these changes, I am getting a (very!) repeated warning for FreeBSD and Linux (Debian):

[  3%] Building CXX object vm/CMakeFiles/Self.dir/src/any/asm/asm.cpp.o
In file included from /root/build/incls/_precompiled.hh:37,
                 from /root/build/incls/_asm.cpp.incl:1,
                 from /self/vm/src/any/asm/asm.cpp:11:
/self/vm/src/any/objects/memOop.hh: In static member function 'static int32 memOopClass::map_offset()':
/self/vm/src/any/objects/memOop.hh:154:37: warning: 'this' pointer is null [-Wnonnull]
  154 |     return int32(&memOop(NULL)->addr()->_map); }
      |                   ~~~~~~~~~~~~~~~~~~^~
/self/vm/src/any/objects/memOop.hh:39:16: note: in a call to non-static member function 'memOopClass* memOopClass::addr()'
   39 |   memOopClass* addr() { return (memOopClass*) (int32(this) - Mem_Tag); }
      |                ^~~~
In file included from /root/build/incls/_precompiled.hh:54:
/self/vm/src/any/objects/byteVectorOop.hh: In static member function 'static int32 byteVectorOopClass::byteVector_len_offset()':
/self/vm/src/any/objects/byteVectorOop.hh:130:41: warning: 'this' pointer is null [-Wnonnull]
  130 |     return int32(&byteVectorOop(0)->addr()->_len); }
      |                   ~~~~~~~~~~~~~~~~~~~~~~^~
/self/vm/src/any/objects/byteVectorOop.hh:24:23: note: in a call to non-static member function 'byteVectorOopClass* byteVectorOopClass::addr()'
   24 |   byteVectorOopClass* addr() {
      |                       ^~~~
/self/vm/src/any/objects/byteVectorOop.hh: In static member function 'static int32 byteVectorOopClass::byteVector_bytes_offset()':
/self/vm/src/any/objects/byteVectorOop.hh:132:41: warning: 'this' pointer is null [-Wnonnull]
  132 |     return int32(&byteVectorOop(0)->addr()->_bytes); }
      |                   ~~~~~~~~~~~~~~~~~~~~~~^~
/self/vm/src/any/objects/byteVectorOop.hh:24:23: note: in a call to non-static member function 'byteVectorOopClass* byteVectorOopClass::addr()'
   24 |   byteVectorOopClass* addr() {
      |                       ^~~~
In file included from /root/build/incls/_precompiled.hh:61:
/self/vm/src/any/objects/objVectorOop.hh: In static member function 'static int32 objVectorOopClass::objVector_len_offset()':
/self/vm/src/any/objects/objVectorOop.hh:81:43: warning: 'this' pointer is null [-Wnonnull]
   81 |     return int32(&objVectorOop(NULL)->addr()->_len); }
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~^~
/self/vm/src/any/objects/objVectorOop.hh:16:22: note: in a call to non-static member function 'objVectorOopClass* objVectorOopClass::addr()'
   16 |   objVectorOopClass* addr() {
      |                      ^~~~
In file included from /root/build/incls/_precompiled.hh:62:
/self/vm/src/any/objects/slotsMapDeps.hh: In static member function 'static int32 slotsMapDeps::map_chain_offset()':
/self/vm/src/any/objects/slotsMapDeps.hh:39:54: warning: 'this' pointer is null [-Wnonnull]
   39 |     return (int32) (((slotsMapDeps*) NULL)->map_chain());
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/self/vm/src/any/objects/slotsMapDeps.hh:20:9: note: in a call to non-static member function 'nmln* slotsMapDeps::map_chain()'
   20 |   nmln *map_chain() { return &_map_chain; }
      |         ^~~~~~~~~
[  4%] Building CXX object vm/CMakeFiles/Self.dir/src/any/asm/disasm.cpp.o

Are you seeing that?

nbuwe commented 1 year ago

Uhm... I'm surprised you didn't see those warnings before. The source code is quite deliberate about it. May be they only kick in with optimizations turned on?

You can see -Wno-nonnull in the list of excluded warnings in the previous comment and -fno-delete-null-pointer-checks was in one of my very first PRs (#135). Thank god gcc still has it. clang folks has drank the kool-aid so recent clang can no longer be used to compile self with optimizations enabled (https://github.com/llvm/llvm-project/issues/59889 - a slightly different but related problem), so e.g. on FreeBSD you have to install gcc from ports.