-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix clang-tidy warnings to make it actually useful #1048
Conversation
@kring When you're back, I believe I've tackled the remaining review comments. |
@@ -708,7 +732,7 @@ void updateExtensionWithJsonStringProperty( | |||
// Because serialized string json will add double quotations in the | |||
// buffer which is not needed by us, we will manually add the string to | |||
// the buffer | |||
const auto& rapidjsonStr = it->IsNull() ? *noDataValue : it->GetString(); | |||
const auto& rapidjsonStr = it->GetString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I look at this code, the more I think it's wrong (both before and after this PR).
The intention of this if/else is, AFAICT, to make sure that property values are shown the same way they would be if serialize to JSON, except that strings should not be quoted. So we should go into the if
when we have a non-string, and into the else when we have a string (so that we can remove the quotes).
Mostly this is straightforward, but there's one tricky bit. If the value is null, and the property has a default value, then we need to use that default value (which is always a string).
But that's not what this code is going to do. If the value is null, then it->IsString()
will be false, and so we'll always go into the if block and never the else. That will end up writing the string null
to the buffer instead of the default value.
I believe the correct code looks roughly like:
if (it->IsString() || (it->IsNull() && noDataValue)) {
// use string formatting that removes quotes
} else {
// Use JSON formatting
}
I'll make this change and push it into this PR.
I've reviewed everything now and it all looks great. Just a couple comments/questions above and this is ready to merge! |
Addressed your comments @kring! |
Thanks @azrogers! 🎉 |
When we first added clang-tidy to Cesium Native, we left the warnings as merely informational, because there were too many of them to fix at the moment. Unfortunately, this leaves clang-tidy as an afterthought, because I don't imagine many of us are poking through the CI logs to see if our changes produced any new clang-tidy warnings when there's already hundreds of existing warnings. This change attempts to fix this: resolve the existing warnings, enable any useful checks that weren't already enabled, and treat the warnings as errors so we'll notice them.
Changes in this PR:
ExcludeHeaderFilterRegex
config option we're using to removeezvcpkg
headers from the build.bugprone
clang-tidy check has been enabled, with the following exceptions:bugprone-exception-escape
, as fixing this is already a pending issue (Functions are declared noexcept, even though they may throw #348).bugprone-misplaced-widening-cast
, as it results in extremely verbose lines (likesize_t pixels = static_cast<size_t>(image.width) * static_cast<size_t>(image.height) * static_cast<size_t>(image.channels)
instead of justsize_t pixels = static_cast<size_t>(image.width * image.height * image.channels)
.bugprone-unhandled-exception-at-new
, as this is both part of Functions are declared noexcept, even though they may throw #348, and also extremely nitpicky - I don't think we need to wrap every call tonew
in atry...catch
.modernize
andmisc
checks have been enabled, where they didn't conflict with the style guide or produce way too many changes for one PR. The following I decided were too much for just this PR, but they can be automatically applied at a later date usingclang-tidy-fix
:misc-const-correctness
detects every variable that could be declaredconst
.modernize-loop-convert
uses range-based for loops instead of manually advancing the iterator.modernize-type-traits
changestraits<...>::type
andtraits<..>::value
totraits_t<...>
andtraits_v<...>
modernize-use-constraints
replacesstd::enable_if
with C++20requires
clauses.modernize-use-using
replacestypedef
withusing
.modernize-return-braced-init-list
replaces statements likereturn glm::dvec2(0.5, 0.5);
withreturn {0.5, 0.5};
modernize-use-designated-initializers
would mean adding.field=
to initializer statements, like{.x = 0.5, .y = 0.5}
, which would prevent bugs when changing the order of fields.performance
checks have been enabled, with the exception ofperformance-enum-size
, as I felt that saving three bytes by defining an enum withint8_t
instead of the defaultint32_t
didn't seem like it was worth the change.readability
checks are mostly style guide-related, but might be worth looking into.readability-else-after-return
andreadability-braces-around-statements
seem like they could be good to add.Fixes #203