[PATCH] D126097: [clang-tidy] Adds the NSDateFormatter checker to clang-tidy

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D126097#3528787 <https://reviews.llvm.org/D126097#3528787>, @NoQ wrote:

> Looks awesome!
>
>> add_new_check.py
>
> I'm surprised it wasn't executable already, do we want to keep it?

It keeps getting reset, likely due to Windows users committing changes. It should be executable but the change shouldn't be made in this patch



================
Comment at: clang-tools-extra/clang-tidy/objc/NsdateformatterCheck.cpp:22
+    // Adding matchers.
+    Finder->addMatcher(objcMessageExpr(hasSelector("setDateFormat:"), hasArgument(0, hasDescendant( stringLiteral().bind("str_lit")))), this);
+}
----------------
NoQ wrote:
> Please clang-format this down to 80 column limit (https://llvm.org/docs/CodingStandards.html).
hasDescendant seems dangerous here. I'm no ObjC expert, but I'm guessing the argument could be a function call, in which case you could match an argument of that nested functionCall that's a StringLiteral.


================
Comment at: clang-tools-extra/clang-tidy/objc/NsdateformatterCheck.cpp:28
+// See: https://www.unicode.org/reports/tr35/tr35.html#Invalid_Patterns
+bool isValidDatePattern(StringRef Pattern) {
+    char ValidDatePatternChars[] = {'G', 'y', 'Y', 'u', 'U', 'r', 'Q', 'q', 'M', 'L', 'I', 'w', 'W', 'd', 'D', 'F', 'g', 'E', 'e', 'c', 'a', 'b', 'B', 'h', 'H', 'K', 'k', 'j', 'J', 'C', 'm', 's', 'S', 'A', 'z', 'Z', 'O', 'v', 'V', 'X', 'x'};
----------------
I'm pretty sure StringRef has a method that does this for you, `contains` I think it's called.


================
Comment at: clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp:18
 #include "NSInvocationArgumentLifetimeCheck.h"
+#include "NsdateformatterCheck.h"
 #include "PropertyDeclarationCheck.h"
----------------
File names for checks should be in CamelCaseFormat.


================
Comment at: clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp:32
+    CheckFactories.registerCheck<NsdateformatterCheck>(
+        "objc-NSDateFormatter");
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
----------------
Eugene.Zelenko wrote:
> I think all check names are lowercase.
Yeah, I'd suggest objc-nsdate-formatter


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:87
    `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
-   `bugprone-shared-ptr-array-mismatch <bugprone-shared-ptr-array-mismatch.html>`_, "Yes"
    `bugprone-signal-handler <bugprone-signal-handler.html>`_,
----------------
NoQ wrote:
> Where did these go?
This is a unintended change from the add_new_check script. It should be reverted here tho.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/objc-NSDateFormatter.m:3
+
+#import <Foundation/Foundation.h>
+
----------------
Again no expert on ObjC, but test files have no system headers so where does this file live?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126097