Fixing clang-tidy errors/warnings #158
Assignees
Comments
|
@springmeyer - is your analyser buggy ^ ?? I'm not getting warnings, maybe I'm missing something. |
|
@artemp your output indicates that only |
|
@artemp Thinking more. The problem is likely that my script to produce |
^ yep, looks rather minimal |
|
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;
^
```
|
|
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: 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:
|
|
@kkaefer I'll take a look, thanks. |
|
More observations:
|
|
quick workaround : struct alignas(1) Empty
{
std::uint8_t val = 1u;
Empty() {};
};I'll see if |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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// NOLINTwhere appropriate or fixing.@artemp can you please tackle this next week?
Here are the warnings:
Here is how I'm testing:
run-clang-tidybranch: https://github.com/mapbox/variant/compare/run-clang-tidy./scripts/clang-tidy.sh/cc @daniel-j-h
The text was updated successfully, but these errors were encountered: