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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
