search

mapbox / variant

C++11/C++14 Variant
BSD 3-Clause "New" or "Revised" License
371 stars 100 forks source link

Fixing clang-tidy errors/warnings #158

Open springmeyer opened 7 years ago

springmeyer commented 7 years ago

I'm attempting to set up clang-tidy to run on codebases using variant. I am consistently seeing most warnings coming from variant. So these need to be addressed here, at the source, by either adding // NOLINT where appropriate or fixing.

@artemp can you please tackle this next week?

Here are the warnings:

``` ./include/mapbox/variant.hpp:344:16: warning: Function call argument is an uninitialized value [clang-analyzer-core.CallAndMessage] return f(unwrapper::apply(v.template get_unchecked())); ^ test/lambda_overload_test.cpp:122:5: note: Calling 'test_match_singleton' test_match_singleton(); ^ test/lambda_overload_test.cpp:87:5: note: Calling 'variant::match' singleton.match([](int) {}); ^ ./include/mapbox/variant.hpp:903:16: note: Calling 'variant::visit' return variant::visit(*this, ::mapbox::util::make_visitor(std::forward(fs)...)); ^ ./include/mapbox/variant.hpp:871:16: note: Calling 'dispatcher::apply' return detail::dispatcher::apply(v, std::forward(f)); ^ ./include/mapbox/variant.hpp:344:18: note: Calling 'unwrapper::apply' return f(unwrapper::apply(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:344:18: note: Returning from 'unwrapper::apply' return f(unwrapper::apply(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:344:16: note: Function call argument is an uninitialized value return f(unwrapper::apply(v.template get_unchecked())); ^ 2 warnings generated. ./include/mapbox/recursive_wrapper.hpp:106:36: warning: Undefined or garbage value returned to caller [clang-analyzer-core.uninitialized.UndefReturn] const T* get_pointer() const { return p_; } ^ test/hashable_test.cpp:157:5: note: Calling 'test_recursive_hashable' test_recursive_hashable(); ^ test/hashable_test.cpp:142:10: note: Calling move constructor for 'variant' Tree tree = Node{Node{Empty{}, Empty{}}, Empty{}}; ^ ./include/mapbox/variant.hpp:615:9: note: Calling 'variant_helper::move' helper_type::move(old.type_index, &old.data, &data); ^ ./include/mapbox/variant.hpp:235:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ ./include/mapbox/variant.hpp:241:13: note: Calling 'variant_helper::move' variant_helper::move(old_type_index, old_value, new_value); ^ ./include/mapbox/variant.hpp:235:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ ./include/mapbox/variant.hpp:241:13: note: Calling 'variant_helper::move' variant_helper::move(old_type_index, old_value, new_value); ^ ./include/mapbox/variant.hpp:241:13: note: Returning from 'variant_helper::move' variant_helper::move(old_type_index, old_value, new_value); ^ ./include/mapbox/variant.hpp:241:13: note: Returning from 'variant_helper::move' variant_helper::move(old_type_index, old_value, new_value); ^ ./include/mapbox/variant.hpp:615:9: note: Returning from 'variant_helper::move' helper_type::move(old.type_index, &old.data, &data); ^ test/hashable_test.cpp:142:10: note: Returning from move constructor for 'variant' Tree tree = Node{Node{Empty{}, Empty{}}, Empty{}}; ^ test/hashable_test.cpp:144:9: note: Calling 'hash::operator()' if (std::hash{}(tree) != ((5 + (5 + (3 + 3))) + 3)) ^ ./include/mapbox/variant.hpp:1022:16: note: Calling 'apply_visitor' return ::mapbox::util::apply_visitor(::mapbox::util::detail::hasher{}, v); ^ ./include/mapbox/variant.hpp:959:12: note: Calling 'variant::visit' return V::visit(v, std::forward(f)); ^ ./include/mapbox/variant.hpp:864:16: note: Calling 'dispatcher::apply_const' return detail::dispatcher::apply_const(v, std::forward(f)); ^ ./include/mapbox/variant.hpp:311:9: note: Taking false branch if (v.template is()) ^ ./include/mapbox/variant.hpp:317:20: note: Calling 'dispatcher::apply_const' return dispatcher::apply_const(v, std::forward(f)); ^ ./include/mapbox/variant.hpp:339:18: note: Calling 'unwrapper::apply_const' return f(unwrapper::apply_const(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:279:16: note: Calling 'recursive_wrapper::get' return obj.get(); ^ ./include/mapbox/recursive_wrapper.hpp:101:17: note: Calling 'recursive_wrapper::get_pointer' return *get_pointer(); ^ ./include/mapbox/recursive_wrapper.hpp:106:36: note: Undefined or garbage value returned to caller const T* get_pointer() const { return p_; } ^ ./include/mapbox/variant.hpp:555:16: warning: Function call argument is an uninitialized value [clang-analyzer-core.CallAndMessage] return std::hash{}(hashable); ^ test/hashable_test.cpp:153:5: note: Calling 'test_singleton' test_singleton(); ^ test/hashable_test.cpp:17:7: note: Calling move constructor for 'variant' V singleton = 5; ^ ./include/mapbox/variant.hpp:615:9: note: Calling 'variant_helper::move' helper_type::move(old.type_index, &old.data, &data); ^ ./include/mapbox/variant.hpp:235:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ ./include/mapbox/variant.hpp:241:13: note: Calling 'variant_helper::move' variant_helper::move(old_type_index, old_value, new_value); ^ ./include/mapbox/variant.hpp:241:13: note: Returning from 'variant_helper::move' variant_helper::move(old_type_index, old_value, new_value); ^ ./include/mapbox/variant.hpp:615:9: note: Returning from 'variant_helper::move' helper_type::move(old.type_index, &old.data, &data); ^ test/hashable_test.cpp:17:7: note: Returning from move constructor for 'variant' V singleton = 5; ^ test/hashable_test.cpp:19:9: note: Calling 'hash::operator()' if (std::hash{}(singleton) != std::hash{}(5)) ^ ./include/mapbox/variant.hpp:1022:16: note: Calling 'apply_visitor' return ::mapbox::util::apply_visitor(::mapbox::util::detail::hasher{}, v); ^ ./include/mapbox/variant.hpp:959:12: note: Calling 'variant::visit' return V::visit(v, std::forward(f)); ^ ./include/mapbox/variant.hpp:864:16: note: Calling 'dispatcher::apply_const' return detail::dispatcher::apply_const(v, std::forward(f)); ^ ./include/mapbox/variant.hpp:339:18: note: Calling 'unwrapper::apply_const' return f(unwrapper::apply_const(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:339:18: note: Returning from 'unwrapper::apply_const' return f(unwrapper::apply_const(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:339:18: note: Passing value via 1st parameter 'hashable' return f(unwrapper::apply_const(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:339:16: note: Calling 'hasher::operator()' return f(unwrapper::apply_const(v.template get_unchecked())); ^ ./include/mapbox/variant.hpp:555:16: note: Function call argument is an uninitialized value return std::hash{}(hashable); ```

Here is how I'm testing:

/cc @daniel-j-h

artemp commented 7 years ago

@springmeyer - is your analyser buggy ^ ?? I'm not getting warnings, maybe I'm missing something.

  LLVM (http://llvm.org/):
  LLVM version 5.0.0svn
  Optimized build.
  Default target: x86_64-apple-darwin16.7.0
  Host CPU: ivybridge
./scripts/clang-tidy.sh
* Already installed at /Users/artem/Projects/mapbox/variant/mason_packages/osx-x86_64/clang-tidy/4.0.1
* Linking /Users/artem/Projects/mapbox/variant/mason_packages/osx-x86_64/clang-tidy/4.0.1
* Links will be inside /Users/artem/Projects/mapbox/variant/mason_packages/.link/
* Using bash fallback for symlinking (install lndir for faster symlinking)
* Done linking /Users/artem/Projects/mapbox/variant/mason_packages/osx-x86_64/clang-tidy/4.0.1
Enabled checks:
    boost-use-to-string
    cert-dcl03-c
    cert-dcl50-cpp
    cert-dcl54-cpp
    cert-dcl59-cpp
    cert-env33-c
    cert-err09-cpp
    cert-err34-c
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp
    cert-fio38-c
    cert-flp30-c
    cert-msc30-c
    cert-msc50-cpp
    cert-oop11-cpp
    clang-analyzer-apiModeling.google.GTest
    clang-analyzer-core.CallAndMessage
    clang-analyzer-core.DivideZero
    clang-analyzer-core.DynamicTypePropagation
    clang-analyzer-core.NonNullParamChecker
    clang-analyzer-core.NullDereference
    clang-analyzer-core.StackAddressEscape
    clang-analyzer-core.UndefinedBinaryOperatorResult
    clang-analyzer-core.VLASize
    clang-analyzer-core.builtin.BuiltinFunctions
    clang-analyzer-core.builtin.NoReturnFunctions
    clang-analyzer-core.uninitialized.ArraySubscript
    clang-analyzer-core.uninitialized.Assign
    clang-analyzer-core.uninitialized.Branch
    clang-analyzer-core.uninitialized.CapturedBlockVariable
    clang-analyzer-core.uninitialized.UndefReturn
    clang-analyzer-cplusplus.NewDelete
    clang-analyzer-cplusplus.NewDeleteLeaks
    clang-analyzer-cplusplus.SelfAssignment
    clang-analyzer-deadcode.DeadStores
    clang-analyzer-llvm.Conventions
    clang-analyzer-nullability.NullPassedToNonnull
    clang-analyzer-nullability.NullReturnedFromNonnull
    clang-analyzer-nullability.NullableDereferenced
    clang-analyzer-nullability.NullablePassedToNonnull
    clang-analyzer-nullability.NullableReturnedFromNonnull
    clang-analyzer-optin.cplusplus.VirtualCall
    clang-analyzer-optin.mpi.MPI-Checker
    clang-analyzer-optin.osx.cocoa.localizability.EmptyLocalizationContextChecker
    clang-analyzer-optin.osx.cocoa.localizability.NonLocalizedStringChecker
    clang-analyzer-optin.performance.Padding
    clang-analyzer-osx.API
    clang-analyzer-osx.NumberObjectConversion
    clang-analyzer-osx.ObjCProperty
    clang-analyzer-osx.SecKeychainAPI
    clang-analyzer-osx.cocoa.AtSync
    clang-analyzer-osx.cocoa.ClassRelease
    clang-analyzer-osx.cocoa.Dealloc
    clang-analyzer-osx.cocoa.IncompatibleMethodTypes
    clang-analyzer-osx.cocoa.Loops
    clang-analyzer-osx.cocoa.MissingSuperCall
    clang-analyzer-osx.cocoa.NSAutoreleasePool
    clang-analyzer-osx.cocoa.NSError
    clang-analyzer-osx.cocoa.NilArg
    clang-analyzer-osx.cocoa.NonNilReturnValue
    clang-analyzer-osx.cocoa.ObjCGenerics
    clang-analyzer-osx.cocoa.RetainCount
    clang-analyzer-osx.cocoa.SelfInit
    clang-analyzer-osx.cocoa.SuperDealloc
    clang-analyzer-osx.cocoa.UnusedIvars
    clang-analyzer-osx.cocoa.VariadicMethodTypes
    clang-analyzer-osx.coreFoundation.CFError
    clang-analyzer-osx.coreFoundation.CFNumber
    clang-analyzer-osx.coreFoundation.CFRetainRelease
    clang-analyzer-osx.coreFoundation.containers.OutOfBounds
    clang-analyzer-osx.coreFoundation.containers.PointerSizedValues
    clang-analyzer-security.FloatLoopCounter
    clang-analyzer-security.insecureAPI.UncheckedReturn
    clang-analyzer-security.insecureAPI.getpw
    clang-analyzer-security.insecureAPI.gets
    clang-analyzer-security.insecureAPI.mkstemp
    clang-analyzer-security.insecureAPI.mktemp
    clang-analyzer-security.insecureAPI.rand
    clang-analyzer-security.insecureAPI.strcpy
    clang-analyzer-security.insecureAPI.vfork
    clang-analyzer-unix.API
    clang-analyzer-unix.Malloc
    clang-analyzer-unix.MallocSizeof
    clang-analyzer-unix.MismatchedDeallocator
    clang-analyzer-unix.StdCLibraryFunctions
    clang-analyzer-unix.Vfork
    clang-analyzer-unix.cstring.BadSizeArg
    clang-analyzer-unix.cstring.NullArg
    cppcoreguidelines-c-copy-assignment-signature
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-no-malloc
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-readability-braces-around-statements
    google-readability-casting
    google-readability-function-size
    google-readability-namespace-comments
    google-readability-redundant-smartptr-get
    google-readability-todo
    google-runtime-int
    google-runtime-member-string-references
    google-runtime-memset
    google-runtime-operator
    google-runtime-references
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-bool-pointer-implicit-conversion
    misc-dangling-handle
    misc-definitions-in-headers
    misc-fold-init-type
    misc-forward-declaration-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const
    misc-misplaced-widening-cast
    misc-move-const-arg
    misc-move-constructor-init
    misc-move-forwarding-reference
    misc-multiple-statement-macro
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert
    misc-string-compare
    misc-string-constructor
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-enum-usage
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-suspicious-string-compare
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-unconventional-assign-operator
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    misc-unused-using-decls
    misc-use-after-move
    misc-virtual-near-miss
    modernize-avoid-bind
    modernize-deprecated-headers
    modernize-loop-convert
    modernize-make-shared
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default-member-init
    modernize-use-emplace
    modernize-use-equals-default
    modernize-use-equals-delete
    modernize-use-nullptr
    modernize-use-override
    modernize-use-transparent-functors
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
    performance-inefficient-string-concatenation
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-misplaced-array-index
    readability-named-parameter
    readability-non-const-parameter
    readability-redundant-control-flow
    readability-redundant-declaration
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

clang-tidy -header-filter=^/Users/artem/Projects/mapbox/variant/build/.* -export-fixes /var/folders/83/rt7rty5j1v3gj5kpymxsq_0w0000gn/T/tmpk73w1f/tmpw0Mi8S.yaml -p=/Users/artem/Projects/mapbox/variant/build /Users/artem/Projects/mapbox/variant/test/bench_variant.cpp
Applying fixes ...
springmeyer commented 7 years ago

@artemp your output indicates that only bench_variant.cpp is being checked. Not sure why that is. Mine is checking more .cpp files locally. Can you share the build/compile_commands.json that is on your machine in a gist? Also, for reference, here is my output https://gist.github.com/springmeyer/86c27ec500c81286bd124128b06f6bd8

springmeyer commented 7 years ago

@artemp Thinking more. The problem is likely that my script to produce compile_commands.json is brittle. It has some minimal regex to try to detect which lines (coming from make) are compile commands: https://github.com/mapbox/variant/blob/bdd45a6aaad0de81f471898dfe958241b618361a/scripts/generate_compile_commands.py#L24

artemp commented 7 years ago 
    {
        "directory": "/Users/artem/Projects/mapbox/variant",
        "command": "/Users/artem/Projects/mapbox/variant/.toolchain/bin/clang++ -o out/bench-variant test/bench_variant.cpp -I./include -isystem test/include -s\
td=c++11 -Werror -Wall -Wextra -pedantic -Wformat=2 -Wsign-conversion -Wshadow -Wunused-parameter -O3 -DNDEBUG -march=native -DSINGLE_THREADED -fvisibility-inli\
nes-hidden -fvisibility=hidden  -stdlib=libc++ -mmacosx-version-min=10.7  -isystem /Users/artem/Projects/mapbox/variant/mason_packages/headers/boost/1.62.0/incl\
ude/",
        "file": "/Users/artem/Projects/mapbox/variant/test/bench_variant.cpp"
    }
]

^ yep, looks rather minimal

kkaefer commented 5 years ago

This is still an issue that prevents us from running clang-tidy on a full codebase. Here's the clang-tidy 7.0.0 message that I'm getting:

``` /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:504:20: error: The right operand of '==' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors] return lhs == rhs; ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:323:9: note: Assuming the condition is false if (it == properties.end()) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:323:5: note: Taking false branch if (it == properties.end()) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:330:9: note: Assuming 'property' is not equal to FillExtrusionOpacity if (property == Property::FillExtrusionOpacity) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:330:5: note: Taking false branch if (property == Property::FillExtrusionOpacity) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:342:9: note: Assuming 'property' is not equal to FillExtrusionColor if (property == Property::FillExtrusionColor) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:342:5: note: Taking false branch if (property == Property::FillExtrusionColor) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:354:9: note: Assuming 'property' is not equal to FillExtrusionTranslate if (property == Property::FillExtrusionTranslate) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:354:5: note: Taking false branch if (property == Property::FillExtrusionTranslate) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:366:9: note: Assuming 'property' is not equal to FillExtrusionTranslateAnchor if (property == Property::FillExtrusionTranslateAnchor) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:366:5: note: Taking false branch if (property == Property::FillExtrusionTranslateAnchor) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:378:9: note: Assuming 'property' is not equal to FillExtrusionPattern if (property == Property::FillExtrusionPattern) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:378:5: note: Taking false branch if (property == Property::FillExtrusionPattern) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:390:9: note: Assuming 'property' is equal to FillExtrusionHeight if (property == Property::FillExtrusionHeight || property == Property::FillExtrusionBase) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:390:51: note: Left side of '||' is true if (property == Property::FillExtrusionHeight || property == Property::FillExtrusionBase) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:393:13: note: Assuming the condition is false if (!typedValue) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:393:9: note: Taking false branch if (!typedValue) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:397:9: note: Taking true branch if (property == Property::FillExtrusionHeight) { ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:398:13: note: Calling 'FillExtrusionLayer::setFillExtrusionHeight' setFillExtrusionHeight(*typedValue); ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:209:18: note: Calling 'FillExtrusionLayer::getFillExtrusionHeight' if (value == getFillExtrusionHeight()) ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:205:12: note: Calling implicit copy constructor for 'PropertyValue' return impl().paint.template get().value; ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:205:12: note: Calling copy constructor for 'variant>' /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:595:9: note: Calling 'variant_helper::copy' helper_type::copy(old.type_index, &old.data, &data); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:234:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:240:13: note: Calling 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:234:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:240:13: note: Calling 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:234:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:240:13: note: Calling 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:250:77: note: Returning without writing to '' VARIANT_INLINE static void copy(const std::size_t, const void*, void*) {} ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:240:13: note: Returning from 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:242:5: note: Returning without writing to 'new_value' } ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:240:13: note: Returning from 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:242:5: note: Returning without writing to 'new_value' } ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:240:13: note: Returning from 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:242:5: note: Returning without writing to 'new_value' } ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:595:9: note: Returning from 'variant_helper::copy' helper_type::copy(old.type_index, &old.data, &data); ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:205:12: note: Returning from copy constructor for 'variant>' return impl().paint.template get().value; ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:205:12: note: Returning from copy constructor for 'PropertyValue' /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:209:18: note: Returning from 'FillExtrusionLayer::getFillExtrusionHeight' if (value == getFillExtrusionHeight()) ^ /Users/kkaefer/Code/gl/native/src/mbgl/style/layers/fill_extrusion_layer.cpp:209:9: note: Calling 'operator==' if (value == getFillExtrusionHeight()) ^ /Users/kkaefer/Code/gl/native/include/mbgl/style/property_value.hpp:22:16: note: Calling 'variant::operator==' return lhs.value == rhs.value; ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:902:9: note: Taking false branch if (this->which() != rhs.which()) ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:907:16: note: Calling 'variant::visit' return visit(rhs, visitor); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:850:16: note: Calling 'dispatcher::apply_const' return detail::dispatcher::apply_const(v, std::forward(f)); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:298:9: note: Taking false branch if (v.template is()) ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:304:20: note: Calling 'dispatcher::apply_const' return dispatcher::apply_const(v, std::forward(f)); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:298:9: note: Taking true branch if (v.template is()) ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:300:20: note: Calling 'comparer::operator()' return f(unwrapper::apply_const(v.template get_unchecked())); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:529:36: note: Passing value via 2nd parameter 'rhs' return Comp()(lhs_content, rhs_content); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:529:16: note: Calling 'equal_comp::operator()' return Comp()(lhs_content, rhs_content); ^ /Users/kkaefer/Code/gl/native/vendor/variant/include/mapbox/variant.hpp:504:20: note: The right operand of '==' is a garbage value return lhs == rhs; ^ ```
kkaefer commented 5 years ago

Here's a minimal reproduction:

#include <mapbox/variant.hpp>

class Empty {};
inline bool operator==(const Empty&, const Empty&) { return true; }

int main() {
    using type = mapbox::util::variant<Empty, int>;
    type a;
    type b = a;
    (void)(a == b);
}

I'm invoking clang-tidy like this:

clang-tidy -checks=clang-analyzer-core.UndefinedBinaryOperatorResult variant.cpp -- -std=c++14 -I<path-to-variant>

and get this output:

``` vendor/variant/include/mapbox/variant.hpp:504:20: error: The right operand of '==' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors] return lhs == rhs; ^ /Users/kkaefer/Code/gl/native/variant.cpp:9:14: note: Calling copy constructor for 'variant' type b = a; ^ vendor/variant/include/mapbox/variant.hpp:595:9: note: Calling 'variant_helper::copy' helper_type::copy(old.type_index, &old.data, &data); ^ vendor/variant/include/mapbox/variant.hpp:234:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ vendor/variant/include/mapbox/variant.hpp:240:13: note: Calling 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ vendor/variant/include/mapbox/variant.hpp:234:9: note: Taking false branch if (old_type_index == sizeof...(Types)) ^ vendor/variant/include/mapbox/variant.hpp:240:13: note: Calling 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ vendor/variant/include/mapbox/variant.hpp:250:77: note: Returning without writing to '' VARIANT_INLINE static void copy(const std::size_t, const void*, void*) {} ^ vendor/variant/include/mapbox/variant.hpp:240:13: note: Returning from 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ vendor/variant/include/mapbox/variant.hpp:242:5: note: Returning without writing to 'new_value' } ^ vendor/variant/include/mapbox/variant.hpp:240:13: note: Returning from 'variant_helper::copy' variant_helper::copy(old_type_index, old_value, new_value); ^ vendor/variant/include/mapbox/variant.hpp:242:5: note: Returning without writing to 'new_value' } ^ vendor/variant/include/mapbox/variant.hpp:595:9: note: Returning from 'variant_helper::copy' helper_type::copy(old.type_index, &old.data, &data); ^ /Users/kkaefer/Code/gl/native/variant.cpp:9:14: note: Returning from copy constructor for 'variant' type b = a; ^ /Users/kkaefer/Code/gl/native/variant.cpp:10:12: note: Calling 'variant::operator==' (void)(a == b); ^ vendor/variant/include/mapbox/variant.hpp:902:9: note: Taking false branch if (this->which() != rhs.which()) ^ vendor/variant/include/mapbox/variant.hpp:907:16: note: Calling 'variant::visit' return visit(rhs, visitor); ^ vendor/variant/include/mapbox/variant.hpp:850:16: note: Calling 'dispatcher::apply_const' return detail::dispatcher::apply_const(v, std::forward(f)); ^ vendor/variant/include/mapbox/variant.hpp:298:9: note: Taking false branch if (v.template is()) ^ vendor/variant/include/mapbox/variant.hpp:304:20: note: Calling 'dispatcher::apply_const' return dispatcher::apply_const(v, std::forward(f)); ^ vendor/variant/include/mapbox/variant.hpp:326:16: note: Calling 'comparer::operator()' return f(unwrapper::apply_const(v.template get_unchecked())); ^ vendor/variant/include/mapbox/variant.hpp:529:36: note: Passing value via 2nd parameter 'rhs' return Comp()(lhs_content, rhs_content); ^ vendor/variant/include/mapbox/variant.hpp:529:16: note: Calling 'equal_comp::operator()' return Comp()(lhs_content, rhs_content); ^ vendor/variant/include/mapbox/variant.hpp:504:20: note: The right operand of '==' is a garbage value return lhs == rhs; ^ ```

Some observations:

artemp commented 5 years ago

@kkaefer I'll take a look, thanks.

kkaefer commented 5 years ago

More observations:

artemp commented 5 years ago

quick workaround : 

struct alignas(1) Empty
{
    std::uint8_t val = 1u;
    Empty() {};
};

I'll see if is_empty<T> can be used to add overloads for empty types.