[PATCH] D55640: [clang-tidy] Implement a check for large Objective-C type encodings 🔍

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added a comment.

In D55640#1329390 <https://reviews.llvm.org/D55640#1329390>, @theraven wrote:

> I wonder if we want to have an option to elide ObjC type info for all non-POD C++ types.  Nothing that you do with the type encoding is likely to be correct (for example, you can see the pointer field in a `std::shared_ptr`, but you can't see that changes to it need to update reference counts) so it probably does more harm than good.


I think it's worth investigating some kind of option for eliding Objective-C type information though I think that investigation is likely beyond the scope of this proposal. Eliding Objective-C type information could change runtime behavior of a variety of systems that rely on this information. This could include systems embedded in Apple's libraries as well as systems implemented in third party or open source libraries. I imagine that an investigation into such a feature would probably need to be quite thorough.



================
Comment at: clang-tidy/objc/TypeEncodingSizeCheck.cpp:36
+      objcPropertyDecl(unless(isExpansionInSystemHeader()),
+                       anyOf(hasAncestor(objcInterfaceDecl().bind("interface")),
+                             hasAncestor(objcCategoryDecl().bind("category"))))
----------------
hokein wrote:
> `hasAncestor` is an expensive matcher, does `hasDeclContext` meet your use cases?
I started looking into using `hasDeclContext` but encountered some unexpected behavior when adding relevant test cases—which should have been added from the beginning—to verify. I am going to continue investigating whether `hasDeclContext` is satisfactory here and try to resolve the unexpected behavior that I encountered.


================
Comment at: clang-tidy/objc/TypeEncodingSizeCheck.cpp:63
+  std::string TypeEncoding;
+  if (const auto *IvarDecl = dyn_cast<ObjCIvarDecl>(EncodedDecl)) {
+    IvarDecl->getASTContext().getObjCEncodingForType(IvarDecl->getType(),
----------------
hokein wrote:
> Do you forget to register the matcher for `ObjCIvarDecl`? In the matcher you register it for `ObjCPropertyDecl`, and `ObjCInterfaceDecl`, so this branch will never be executed.
On line 32 in this diff is where I register the matcher for `ObjCIvarDecl`. The matching is functioning as I expect it should. Let me know if you want me to reorganize the matching logic.


================
Comment at: docs/clang-tidy/checks/objc-type-encoding-size.rst:6
+
+Finds Objective-C type encodings that exceed a configured threshold.
+
----------------
Eugene.Zelenko wrote:
> Please synchronize with Release Notes.
I changed the check description to match the release notes. Let me know if I misunderstood the requested change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55640/new/

https://reviews.llvm.org/D55640