[clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

[stephanemoore]

Summary

In contrast to Google C++, Objective-C often uses built-in integer types
other than int. In fact, the Objective-C runtime itself defines the
types NSInteger¹ and NSUInteger² which are variant types depending on
the target architecture. The Objective-C style guide indicates that
usage of system types with variant sizes is appropriate when handling
values provided by system interfaces³. Objective-C++ is commonly the
result of conversion from Objective-C to Objective-C++ for the purpose
of integrating C++ functionality. The opposite of Objective-C++ being
used to expose Objective-C functionality to C++ is less common,
potentially because Objective-C has a signficantly more uneven presence
on different platforms compared to C++. This generally predisposes
Objective-C++ to commonly being more Objective-C than C++. Forcing
Objective-C++ developers to perform conversions between variant system types
and fixed size integer types depending on target architecture when
Objective-C++ commonly uses variant system types from Objective-C is
likely to lead to more bugs and overhead than benefit. For that reason,
this change proposes to disable google-runtime-int in Objective-C++.

[1] https://developer.apple.com/documentation/objectivec/nsinteger?language=objc
[2] https://developer.apple.com/documentation/objectivec/nsuinteger?language=objc
[3] "Types long, NSInteger, NSUInteger, and CGFloat vary in size between
32- and 64-bit builds. Use of these types is appropriate when handling
values exposed by system interfaces, but they should be avoided for most
other computations."
https://github.com/google/styleguide/blob/gh-pages/objcguide.md#types-with-inconsistent-sizes

[stephanemoore]

Document change in release notes.

[stephanemoore]

Let me know if you think this change would benefit from added tests.

[gribozavr]

clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp

57

"Objective-C and Objective-C++"? Similarly in release notes.

[gribozavr]

clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp

57

I see -- should have read the previous sentence, which you are not changing. Never mind.

[stephanemoore]

Thanks for the review!

clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp

57

Yup, the previous sentence indicates that the check targets C++ and this added sentence indicates that Objective-C++ is considered an exception.

[aaron.ballman]

This is missing test cases that demonstrate the behavior is what we expect it to be for ObjC++ code vs C++ code.

[lebedev.ri]

In D59336#1429564, @aaron.ballman wrote:

This is missing test cases that demonstrate the behavior is what we expect it to be for ObjC++ code vs C++ code.

Looks like phab has again consumed the email comments.
Might be a good idea to add the backlogged test coverage too.

https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190311/264471.html

test?

I was uncertain whether or not this change required new tests. A previous change which disabled this check in languages other than C++ did not include additional tests:
https://github.com/llvm/llvm-project/commit/ec3e5d6fd87862eb77a2b0320d79b9a4427d39df#diff-a491be84e1b831aeaea56c39b5eb898c
If there is a preference to add tests for this change, I can do so.

[stephanemoore]

Add google-runtime-int tests for Objective-C and Objective-C++.

[stephanemoore]

In D59336#1429564, @aaron.ballman wrote:

This is missing test cases that demonstrate the behavior is what we expect it to be for ObjC++ code vs C++ code.

Added tests for Objective-C and Objective-C++.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/google-runtime-int.m

1 ↗

(On Diff #190742)

We typically use | count 0 instead of an explicit grep. Also, do you really need to pass -x objective-c?

clang-tools-extra/test/clang-tidy/google-runtime-int.mm

1 ↗

(On Diff #190742)

Given that the contents of the file are the same, I would just add this RUN line to the previous test case (changing to | count 0). You will still need the -x objective-c++ in that case.

[stephanemoore]

Updated with changes:
• Switched to using | count 0 instead of grep'ing for warnings.
• Merged Objective-C and Objective-C++ test cases into single file with multiple runs.

[stephanemoore]

clang-tools-extra/test/clang-tidy/google-runtime-int.m

1 ↗

(On Diff #190742)

I had chosen to use grep and had explicitly specified the language to be consistent with google-runtime-int.c. I agree that | count 0 is more common and have switched to that approach.

I thought that I should be able to remove -x objective-c but when I try to I get an error loading the compilation database:

Error while trying to load a compilation database:
Could not auto-detect compilation database for file "path_to_llvm_project/clang-tools-extra/test/clang-tidy/google-runtime-int.m"
No compilation database found in path_to_llvm_project/clang-tools-extra/test/clang-tidy or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.

I am still investigating the underlying reason.

clang-tools-extra/test/clang-tidy/google-runtime-int.mm

1 ↗

(On Diff #190742)

Makes sense. Done.

[stephanemoore]

Remove redundant specification of -x objective-c.

[stephanemoore]

clang-tools-extra/test/clang-tidy/google-runtime-int.m

1 ↗

(On Diff #190742)

Apparently it was because I removed the -- as well 😅 I have now removed -x objective-c.

[aaron.ballman]

LGTM!

[stephanemoore]

Thanks for the review!

clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp

       TypeSuffix(Options.get("TypeSuffix", "")) {}
 
 void IntegerTypesCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "UnsignedTypePrefix", UnsignedTypePrefix);
   Options.store(Opts, "SignedTypePrefix", SignedTypePrefix);
   Options.store(Opts, "TypeSuffix", TypeSuffix);
 }
 
 void IntegerTypesCheck::registerMatchers(MatchFinder *Finder) {
   // Find all TypeLocs. The relevant Style Guide rule only applies to C++.
-  if (!getLangOpts().CPlusPlus)
+  // This check is also not applied in Objective-C++ sources as Objective-C
+  // often uses built-in integer types other than `int`.
+  if (!getLangOpts().CPlusPlus || getLangOpts().ObjC)
     return;
   // Match any integer types, unless they are passed to a printf-based API:
   //
   // http://google.github.io/styleguide/cppguide.html#64-bit_Portability
   // "Where possible, avoid passing arguments of types specified by
   // bitwidth typedefs to printf-based APIs."
   Finder->addMatcher(typeLoc(loc(isInteger()),
                              unless(hasAncestor(callExpr(
                                  callee(functionDecl(hasAttr(attr::Format)))))))
                          .bind("tl"),

clang-tools-extra/docs/ReleaseNotes.rst

   <clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>` to
   :doc:`modernize-use-override
   <clang-tidy/checks/modernize-use-override>` was added.
 
 - The :doc:`bugprone-argument-comment
   <clang-tidy/checks/bugprone-argument-comment>` now supports
   `CommentBoolLiterals`, `CommentIntegerLiterals`, `CommentFloatLiterals`,
   `CommentUserDefiniedLiterals`, `CommentStringLiterals`,
   `CommentCharacterLiterals` & `CommentNullPtrs` options.
 
+- The :doc:`google-runtime-int <clang-tidy/checks/google-runtime-int>`
+  check has been disabled in Objective-C++.
+
 - The `Acronyms` and `IncludeDefaultAcronyms` options for the
   :doc:`objc-property-declaration <clang-tidy/checks/objc-property-declaration>`
   check have been removed.
 
 - The :doc:`modernize-use-override
   <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
 Improvements to include-fixer
 -----------------------------