Page MenuHomePhabricator

[clang-tidy] New check calling out uses of +new in Objective-C code
ClosedPublic

Authored by mwyman on Apr 30 2019, 3:31 PM.

Diff Detail

Event Timeline

mwyman created this revision.Apr 30 2019, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 3:31 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
29 ↗(On Diff #197463)

Please use static instead anonymous namespace for functions. See LLVM Coding Guidelines. Same for other functions.

clang-tools-extra/docs/ReleaseNotes.rst
107 ↗(On Diff #197463)

Please highlight +new with double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
4 ↗(On Diff #197463)

Length should be same as title.

6 ↗(On Diff #197463)

Please synchronize first statement with Release Notes.

9 ↗(On Diff #197463)

Please highlight +alloc and -init with double back-ticks.

19 ↗(On Diff #197463)

Please highlight +alloc/-init with double back-ticks.

Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko retitled this revision from New clang-tidy check calling out uses of +new in Objective-C code to [clang-tidy] New check calling out uses of +new in Objective-C code.
mwyman updated this revision to Diff 197660.May 1 2019, 4:45 PM
mwyman marked 6 inline comments as done.

Fixes per review comments.

Eugene.Zelenko added inline comments.May 1 2019, 4:53 PM
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
16 ↗(On Diff #197660)

Unnecessary empty line.

29 ↗(On Diff #197660)

Please elide braces.

34 ↗(On Diff #197660)

Please elide braces.

46 ↗(On Diff #197660)

Please elide braces.

94 ↗(On Diff #197660)

You could return {}

115 ↗(On Diff #197660)

Please elide braces.

clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
27 ↗(On Diff #197660)

I think you could format link in cert-env33-c.rst style.

mwyman updated this revision to Diff 197681.May 1 2019, 5:45 PM
mwyman marked 7 inline comments as done.

Updated per review comments.

alexfh edited reviewers, added: gribozavr, ilya-biryukov; removed: hokein, alexfh.May 7 2019, 5:51 AM
aaron.ballman added inline comments.May 7 2019, 6:42 AM
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
73 ↗(On Diff #197681)

We don't typically make local value types const (only pointers or references); here and elsewhere in the patch.

76–77 ↗(On Diff #197681)

Should this be configurable, or will users not need to control the behavior here?

mwyman updated this revision to Diff 198495.May 7 2019, 10:02 AM

Updated to address code review comments.

mwyman marked 3 inline comments as done.May 7 2019, 10:04 AM
mwyman added inline comments.
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
76–77 ↗(On Diff #197681)

For now I think there is only this handful of Foundation types commonly created only with the factory methods; I'm not sure it makes a huge amount of sense to open it for configuration at this time.

gribozavr accepted this revision.May 7 2019, 10:18 AM
gribozavr added inline comments.
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
55 ↗(On Diff #198495)

"Returns"

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h
19 ↗(On Diff #198495)

It is unclear whether the check ensures that the code does not use +new, or whether it finds code that does not use +new, this description can be read both ways.

Try: This check finds Objective-C code that ...

clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
6 ↗(On Diff #198495)

"Finds calls to ..."

This revision is now accepted and ready to land.May 7 2019, 10:18 AM
aaron.ballman accepted this revision.May 7 2019, 11:29 AM
aaron.ballman marked an inline comment as done.

LGTM modulo the comments from @gribozavr.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
76–77 ↗(On Diff #197681)

That seems reasonable to me -- we can add an option later if we find we need one.

mwyman updated this revision to Diff 198503.May 7 2019, 11:30 AM
mwyman marked an inline comment as done.

Update comments/doc based on feedback.

mwyman marked 3 inline comments as done.May 7 2019, 11:34 AM
stephanemoore accepted this revision.May 7 2019, 12:41 PM

Looks good to me once the comments from other reviewers have been addressed. I added a couple suggestions as well.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
112 ↗(On Diff #198503)

If the message expression is within a macro expansion, maybe we should emit the diagnostic without the fixit?

clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
1 ↗(On Diff #198503)

Suggestion:
It might be good to add a test case for another root class that doesn't have -init, e.g., NSProxy.

mwyman updated this revision to Diff 198702.May 8 2019, 11:12 AM
mwyman marked 2 inline comments as done.

Update for review comments.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
112 ↗(On Diff #198503)

I'm leery of emitting a warning in a case where the code may not originate from the code in question. Maybe if the macro is defined in the same source file, but I think perhaps that could be a follow-up.

mwyman accepted this revision.May 10 2019, 1:30 PM

I don't have commit access, so if somebody could submit them that would be wonderful!

stephanemoore accepted this revision.May 13 2019, 3:07 PM

I believe that all the feedback from @gribozavr has been addressed modulo one small nit. Once that nit has been addressed, I can land this.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h
19 ↗(On Diff #198702)

I believe @gribozavr recommended "This check".

mwyman updated this revision to Diff 199338.May 13 2019, 3:08 PM

Update for comments

mwyman marked an inline comment as done.May 13 2019, 3:09 PM
stephanemoore requested changes to this revision.May 13 2019, 3:27 PM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
112 ↗(On Diff #198503)

I think that it's reasonable to surface the diagnostic. If the user determines that the diagnostic is not relevant in this situation then they can suppress it. That is, I believe it's preferable to surface issues and have the user determine relevance rather than suppress diagnostics without the knowledge of the user.

(with that said, I do not consider this a hard blocker and am comfortable deferring this for future consideration)

clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
53 ↗(On Diff #199338)

Shouldn't you declare +[ProxyFoo new] so that you can call it on line 67?

This revision now requires changes to proceed.May 13 2019, 3:27 PM
mwyman updated this revision to Diff 199341.May 13 2019, 3:36 PM

Added +new declaration for ProxyFoo.

mwyman marked an inline comment as done.May 13 2019, 3:37 PM
mwyman updated this revision to Diff 199375.May 14 2019, 12:45 AM

Bah, previous changes not caught in Git commit; switching back and forth between Git/Mercurial makes for some mix-ups, I guess.

stephanemoore accepted this revision.May 14 2019, 3:08 PM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
54 ↗(On Diff #199375)

It is a little odd that "google-objc-avoid-nsobject-new" warns on +new even for classes that are not derived from NSObject. In a sense, "nsobject-new" is being used as an identifier for any class method named "new". Interestingly, renaming to the more direct "google-objc-avoid-new-class-method" is even more misleading. I have yet to think of a name that I think would actually be better.

While I think the current name is potentially misleading, I think I am okay with this name for the following reasons:
(1) I believe this will only be misleading in marginal scenarios. I believe that it will be exceedingly rare for a class method named new to exist in a class hierarchy where the root class is not NSObject.
(2) We can always amend the check to restrict it to NSObject. In the unexpected circumstance where someone does provide a legitimate use case for a class method named new in a class hierarchy where the root class is not NSObject, we can simply amend the check to restrict its enforcement to class hierarchies where the root class is NSObject.

This revision is now accepted and ready to land.May 14 2019, 3:08 PM
mwyman updated this revision to Diff 200824.May 22 2019, 2:45 PM

Syncing code with HEAD

This revision was automatically updated to reflect the committed changes.