-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Disable google-readability-casting
as it is broken.
#3958
Conversation
I had been bothered by the confusing suggestions to replace calls to a constructor with ... calls to a constructor, claiming the original code was a C-style cast. In some code, we've worked around this by using `static_cast` but that really doesn't seem to be a readability win. I did some research and from what I can tell this tidy check is deeply broken, with both Chromium and internal Google users disabling it due to widespread issues: https://chromium.googlesource.com/chromium/src/+/f828a2248f0da4e7bfadfa5db324230043aaea02 We should disable it as well for now. I've removed one `NOLINT` comment for it, but not tried to remove all the `static_cast`s that are now unnecessary as that seems hard to do in any automatic way.
Isn't this tidy check only warning about C-style casts? i.e., from a style perspective, doesn't Google's casting style still apply? If there are false positives for constructors, that would change. But we would still frequently be casting due to things like int vs size_t comparisons (which are probably the most frequent case). As a consequence, That is to say, we have the occasional false positive, but also we use C++ casts a lot. If someone unfamiliar with Google C++ style comes along and starts adding C-style casts, is it going to be a net improvement if we're correcting the style in review? Should style change to instead encourage C-style casts? |
@@ -21,6 +21,10 @@ WarningsAsErrors: '*' | |||
# inst_id = name_ref->value_id; | |||
# ^ unchecked access to optional value | |||
# } | |||
# - google-readability-casting is deeply broken, and fires regularly on calls |
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.
If we're moving forward with this, is there an issue you can link to?
# - google-readability-casting is deeply broken, and fires regularly on calls | ||
# to constructors. Worse, it suggests switching to a call to a constructor | ||
# which doesn't fix the warning. This has been disabled by many major users | ||
# of clang-tidy (Chromium, etc). |
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.
Note, the way you're talking about this issue is pretty strong. Would it be worth softening up the wording for other readers? e.g., maybe we could focus on the issue as it affects us:
# - google-readability-casting is deeply broken, and fires regularly on calls | |
# to constructors. Worse, it suggests switching to a call to a constructor | |
# which doesn't fix the warning. This has been disabled by many major users | |
# of clang-tidy (Chromium, etc). | |
# - google-readability-casting fires regularly on calls to constructors. It suggests switching to a call to a constructor, which doesn't fix the warning. |
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.
Note too, you might also consider rephrasing the PR title and description.
I should also ask here, is this also an issue with [ed: to be sure -- it's not clear to me from https://crbug.com/1282228 whether they considered brace-based init as a possible solution] |
Well, after a huge debugging session that should have been much shorter (Jon told me exactly what command I should run to confirm my assumption, and I took way to long to run it), I understand the issue better now. There was some bug at some point or in some patched versions of At some point, I was sure there weren't other versions of |
I'm still seeing problems with this check in our CI. Should we disable the check? |
Following discussions around #3958, try to provide more specific semantics. Note we currently don't follow this everywhere, particularly in AddInst calls, but the intent is to shift. Per discussion, designated initializers are preferred when possible. And the `google-readability-casting` diagnostics are poor, but with this may primarily flag cases which should be using a different constructor syntax, so are just a rocky way to get there. --------- Co-authored-by: Richard Smith <richard@metafoo.co.uk>
I had been bothered by the confusing suggestions to replace calls to a constructor with ... calls to a constructor, claiming the original code was a C-style cast. In some code, we've worked around this by using
static_cast
but that really doesn't seem to be a readability win.I did some research and from what I can tell this tidy check is deeply broken, with both Chromium and internal Google users disabling it due to widespread issues:
https://chromium.googlesource.com/chromium/src/+/f828a2248f0da4e7bfadfa5db324230043aaea02
We should disable it as well for now. I've removed one
NOLINT
comment for it, but not tried to remove all thestatic_cast
s that are now unnecessary as that seems hard to do in any automatic way.Replace this paragraph with a description of what this PR is changing or
adding, and why.
Closes #ISSUE