Reports incorrect string patterns in the date format specifier.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks awesome!
add_new_check.py
I'm surprised it wasn't executable already, do we want to keep it?
clang-tools-extra/clang-tidy/objc/CMakeLists.txt | ||
---|---|---|
13 | Looks like everybody's respecting CaPiTaLiZaTiOn, I guess we could too? | |
clang-tools-extra/clang-tidy/objc/NsdateformatterCheck.cpp | ||
22 ↗ | (On Diff #431076) | Please clang-format this down to 80 column limit (https://llvm.org/docs/CodingStandards.html). |
clang-tools-extra/docs/clang-tidy/checks/list.rst | ||
38–39 | Where did these go? |
clang-tools-extra/clang-tidy/objc/NsdateformatterCheck.cpp | ||
---|---|---|
12 ↗ | (On Diff #431076) | cctype? See modernize-deprecated-headers. |
30 ↗ | (On Diff #431076) | Range loop? See modernize-loop-convert. |
clang-tools-extra/clang-tidy/objc/NsdateformatterCheck.h | ||
29 ↗ | (On Diff #431076) | Please add isLanguageVersionSupported. |
clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp | ||
32 | I think all check names are lowercase. | |
34 | Excessive newline. | |
clang-tools-extra/docs/clang-tidy/checks/objc-NSDateFormatter.rst | ||
5 ↗ | (On Diff #431076) | Please separate with newline. |
10 ↗ | (On Diff #431076) | Please enclose all examples and formats into single back-ticks. |
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
clang-tools-extra/clang-tidy/objc/NsdateformatterCheck.cpp | ||
---|---|---|
22 ↗ | (On Diff #431076) | 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. |
28 ↗ | (On Diff #431076) | I'm pretty sure StringRef has a method that does this for you, contains I think it's called. |
clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp | ||
19 | File names for checks should be in CamelCaseFormat. | |
32 | Yeah, I'd suggest objc-nsdate-formatter | |
clang-tools-extra/docs/clang-tidy/checks/list.rst | ||
38–39 | This is a unintended change from the add_new_check script. It should be reverted here tho. | |
clang-tools-extra/test/clang-tidy/checkers/objc-NSDateFormatter.m | ||
3 ↗ | (On Diff #431076) | Again no expert on ObjC, but test files have no system headers so where does this file live? |
Clang-Tidy tests and docs have moved to module subdirectories.
Please rebase this onto main:HEAD and:
- fold your changes into the appropriate subdirs, stripping the module prefix from new files.
- make the target check-clang-extra to validate your tests
- make the target docs-clang-tools-html to validate any docs changes
Uh-oh sorry! I have a couple more questions and then we can probably land.
clang-tools-extra/clang-tidy/objc/NSDateFormatterCheck.cpp | ||
---|---|---|
25 | Can or should we check the class name as well? It's probably going to be pretty rare that people have a method with the same name, that also accepts the first argument as a string literal, but that literal means something else entirely. However there's little reason not to be more cautious about that. | |
68–69 | I think we want spaces in such cases. I'm not sure, discussion welcome. |
Great, thanks! Unless there are other objections from other reviewers, I think this is good to go.
Looks like everybody's respecting CaPiTaLiZaTiOn, I guess we could too?