Fix "missing a nullability type specifier" build warning

[yingsu00]

This PR fixes the "missing a nullability type specifier" warning thrown during the build process, so that the build messages get cleaner. The following message no longer appear after this fix:

In file included from /Users/yingsu/repo/velox1/velox/velox/functions/sparksql/tests/MapTest.cpp:17:
In file included from /Users/yingsu/repo/velox1/velox/./velox/functions/prestosql/tests/FunctionBaseTest.h:20:
In file included from /Users/yingsu/repo/velox1/velox/./velox/expression/Expr.h:24:
In file included from /Users/yingsu/repo/velox1/velox/./velox/core/Expressions.h:20:
In file included from /Users/yingsu/repo/velox1/velox/./velox/vector/BaseVector.h:29:
In file included from /Users/yingsu/repo/velox1/velox/./velox/buffer/Buffer.h:26:
/Users/yingsu/repo/velox1/velox/./velox/common/memory/Memory.h:185:17: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
  void free(void* p, int64_t size) override {
                ^
/Users/yingsu/repo/velox1/velox/./velox/common/memory/Memory.h:185:17: note: insert '_Nullable' if the pointer may be null
  void free(void* p, int64_t size) override {
                ^
                  _Nullable
/Users/yingsu/repo/velox1/velox/./velox/common/memory/Memory.h:185:17: note: insert '_Nonnull' if the pointer should never be null
  void free(void* p, int64_t size) override {
                ^
                  _Nonnull
In file included from /Users/yingsu/repo/velox1/velox/velox/functions/sparksql/tests/MapTest.cpp:17:
In file included from /Users/yingsu/repo/velox1/velox/./velox/functions/prestosql/tests/FunctionBaseTest.h:20:
In file included from /Users/yingsu/repo/velox1/velox/./velox/expression/Expr.h:24:
In file included from /Users/yingsu/repo/velox1/velox/./velox/core/Expressions.h:20:
In file included from /Users/yingsu/repo/velox1/velox/./velox/vector/BaseVector.h:40:
In file included from /Users/yingsu/repo/velox1/velox/./velox/vector/VectorStream.h:19:
In file included from /Users/yingsu/repo/velox1/velox/./velox/common/memory/ByteStream.h:18:
In file included from /Users/yingsu/repo/velox1/velox/./velox/common/memory/StreamArena.h:17:
In file included from /Users/yingsu/repo/velox1/velox/./velox/common/memory/MappedMemory.h:29:
/Users/yingsu/repo/velox1/velox/./velox/common/time/Timer.h:43:11: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Wnullability-completeness]
  uint64_t* timer_;
          ^
/Users/yingsu/repo/velox1/velox/./velox/common/time/Timer.h:43:11: note: insert '_Nullable' if the pointer may be null
  uint64_t* timer_;
          ^
            _Nullable
/Users/yingsu/repo/velox1/velox/./velox/common/time/Timer.h:43:11: note: insert '_Nonnull' if the pointer should never be null
  uint64_t* timer_;
          ^
            _Nonnull
2 warnings generated.

[yingsu00]

Build failure was due to #2029

[yingsu00]

Can anyone review this and merge it? These warnings are pretty annoying.

[mbasmanova]

@yingsu00 I believe this was fixed in #1944, no?

[yingsu00]

@yingsu00 I believe this was fixed in #1944, no?

@mbasmanova #1944 only works when ENABLE_ALL_WARNINGS is defined. By default this variable is OFF, so the warnings were output in default. See the pic:

Furthermore, that PR causes build to fail in some build environments:

cc1plus: error: unrecognized command line option '-Wno-nullability-completeness' [-Werror]
cc1plus: all warnings being treated as errors

See https://app.circleci.com/pipelines/github/facebookincubator/velox/10450/workflows/6da37dd9-8401-46d4-b53b-5315f472c30d/jobs/60413

Lastly, I think this PR is a better fix than #1944, since the warnings are all from the two places and the nullability could be easily deduced. For both incurrence, the nullability was already defined in other places. For example:

class MicrosecondTimer {
 public:
  explicit MicrosecondTimer(uint64_t* FOLLY_NONNULL timer) : timer_(timer) {.  // constructor says timer is gonna be nonnull
    start_ = std::chrono::steady_clock::now();
  }

  ~MicrosecondTimer() {
    auto duration = std::chrono::duration_cast<std::chrono::microseconds>(
        std::chrono::steady_clock::now() - start_);

    (*timer_) += duration.count();
  }

 private:
  std::chrono::steady_clock::time_point start_;
  uint64_t* timer_;   // insert _Nonnull here
};

I think we should revert #1944 and merge this one.

[mbasmanova]

CC: @laithsakka Laith, do you have an opinion?

[yingsu00]

@mbasmanova @laithsakka I removed the -Wno-nullability-completeness and added fix to the newly merged DwrfData.h/cpp in this PR. I don't see any warnings in my CLion as well as CircleCI, so it seems -Wno-nullability-completeness is actually not needed. Also it's blocking the native Parquet reader PR because it's not supported by the compiler in the storage adapter CI build.

[pedroerp]

@yingsu00 it looks like ENABLE_ALL_WARNINGS is always enabled in the Makefile (unless you explicitly disable it). Are you not using the Makefile to compile it?

Furthermore, that PR causes build to fail in some build environments:

Is this because this flag only exists in clang, in which case we should move the flag to the clang-specific branch right above?

[laithsakka]

I think let's step out and decide on the topic on what is the benefit of enabling this warning and adding those annotations?
does it affect perf?
does affect testing and sanitization?
or is it just to inform programmers if a pointer can be null?

@pedroerp @mbasmanova

https://discourse.llvm.org/t/rfc-nullability-qualifiers/35672
FOLLY_NONNULL used 243 times in velox.
FOLLY_NULLABLE used 181 times in velox

The macros here simply silence the warning.

#if FOLLY_HAS_EXTENSION(nullability)
#define FOLLY_NULLABLE                                   \
  FOLLY_PUSH_WARNING                                     \
  FOLLY_CLANG_DISABLE_WARNING("-Wnullability-extension") \
  _Nullable FOLLY_POP_WARNING
#define FOLLY_NONNULL                                    \
  FOLLY_PUSH_WARNING                                     \
  FOLLY_CLANG_DISABLE_WARNING("-Wnullability-extension") \
  _Nonnull FOLLY_POP_WARNING
#else
#define FOLLY_NULLABLE
#define FOLLY_NONNULL
#endif

[laithsakka]

Reading it sounds like for now it's just informative to the programmer, One interesting thing I found is that not all nulls in Velox code are annotated. Yer the compiler does not always complain about all of them!?

[laithsakka]

we use 400 times 4 more is ok.
but i wonder why the compiler generate it from places but not others where pointers are used. lol if this thing end up being annoying and flaky(locations already there not complained about before being complained about again, I would siable the warrning again)

[yingsu00]

it looks like ENABLE_ALL_WARNINGS is always enabled in the Makefile (unless you explicitly disable it). Are you not using the Makefile to compile it?

@pedroerp Pedro, the CLion does not set ENABLE_ALL_WARNINGS to run tests. It runs command like this:

/Applications/CLion.app/Contents/bin/cmake/mac/bin/cmake --build /Users/yingsu/repo/velox6/velox/cmake-build-debug --target velox_dwio_dwrf_writer_test -- -j 12

Is this because this flag only exists in clang, in which case we should move the flag to the clang-specific branch right above?

I think so.

[Yuhta]

Merged with bc6de77