This is an archive of the discontinued LLVM Phabricator instance.

add check to avoid throwing objc exception according to Google Objective-C guide
ClosedPublic

Authored by Wizard on Nov 14 2017, 5:02 PM.

Event Timeline

Wizard created this revision.Nov 14 2017, 5:02 PM
Wizard edited the summary of this revision. (Show Details)Nov 14 2017, 5:05 PM
Wizard added reviewers: hokein, benhamilton.
Wizard added a subscriber: cfe-commits.
Wizard updated this revision to Diff 122953.Nov 14 2017, 5:06 PM

new line

hokein accepted this revision.Nov 15 2017, 12:46 AM

LGTM with a few nits, I'd like to see whether @benhamilton has comments before committing it.

docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst
5

nit: remove the extra ===

11
test/clang-tidy/google-objc-avoid-throwing-exception.m
12

nit: remove the empty line.

This revision is now accepted and ready to land.Nov 15 2017, 12:46 AM
benhamilton requested changes to this revision.Nov 15 2017, 8:24 AM
This comment was removed by benhamilton.
clang-tidy/google/AvoidThrowingObjcExceptionCheck.cpp
23

It looks like we also need to catch (no pun intended :) people who invoke the following class methods:

+[NSException raise:format:]
+[NSException raise:format:arguments:]

We actually have a few places that use these:

http://google3/googlemac/iPhone/Maps/SDK/Maps/GMSServices.mm?l=412&rcl=174266104
http://google3/googlemac/iPhone/Bigtop/Source/Actions/ActionHandler.m?l=1942&rcl=174353485
http://google3/googlemac/iPhone/Applecrisp/Sketchy/Classes/Sketchy/SketchyLib/SketchyApplicationContext.mm?l=398&rcl=175173348

31

Suggested wording:

"Pass in NSError ** instead of using @throw to indicate Objective-C errors"

clang-tidy/google/AvoidThrowingObjcExceptionCheck.h
2

Naming nit-pick: We are currently using ObjC, not Objc.

22–23

Grammar nit:

according to Google Objective-C Style Guide -> according to the Google Objective-C Style Guide

27

Objc -> ObjC

docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst
7–8

How about:

This check finds @throw usages in Objective-C files. For the same reasons as the Google C++ style guide, we prefer not throwing exceptions from Objective-C code.

Instead, prefer passing in NSError ** and return BOOL to indicate success or failure. A counterexample:

- (void)readFile {
  if ([self isError]) {
    @throw [NSException exceptionWithName:...];
  }
}

Instead, returning an error via NSError ** is preferred:

- (BOOL)readFileWithError:(NSError **)error {
  if ([self isError]) {
    *error = [NSError errorWithDomain:...];
    return NO;
  }
  return YES;
}
11

+1

This revision now requires changes to proceed.Nov 15 2017, 8:24 AM
hokein added inline comments.Nov 15 2017, 8:47 AM
clang-tidy/google/AvoidThrowingObjcExceptionCheck.h
2

We also need to rename the file name.

docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst
7–8

+1 on an illustrated sample :)

Wizard updated this revision to Diff 123079.Nov 15 2017, 2:06 PM
Wizard edited edge metadata.
Wizard marked 10 inline comments as done.

address comments

Wizard marked 2 inline comments as done.Nov 15 2017, 2:08 PM
benhamilton accepted this revision.Nov 15 2017, 2:33 PM

Looking good. Please rename the files correctly, or it will not build on Linux.

clang-tidy/google/AvoidThrowingObjcExceptionCheck.cpp
24

Also need to match raise:format:arguments:.

clang-tidy/google/AvoidThrowingObjcExceptionCheck.h
2

Don't forget to rename the file to AvoidThrowingObjcExceptionCheck.h.

clang-tidy/google/CMakeLists.txt
5

Rename to AvoidThrowingObjCExceptionCheck.cpp

clang-tidy/google/GoogleTidyModule.cpp
18

I think you renamed the #include but not the file.. should test on Linux, it won't work with a case-sensitive file system ;)

docs/ReleaseNotes.rst
63

detect usage of @throw -> detect throwing exceptions

docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst
7

Can you include the link to the C++ style guide section here instead of at the end? http://google.github.io/styleguide/cppguide.html#Exceptions

7

finds @throw usages -> finds uses of throwing exceptions

This revision is now accepted and ready to land.Nov 15 2017, 2:33 PM
benhamilton added inline comments.Nov 15 2017, 2:34 PM
clang-tidy/google/AvoidThrowingObjcExceptionCheck.h
2

Oops! I mean, rename the file to AvoidThrowingObjCExceptionCheck.h. :)

test/clang-tidy/google-objc-avoid-throwing-exception.m
18–21

Would be nice to have a test that confirms sending -raise:format: to a different object does not hit the warning.

Wizard updated this revision to Diff 123089.Nov 15 2017, 2:58 PM
Wizard marked 2 inline comments as done.

address comments

Wizard updated this revision to Diff 123090.Nov 15 2017, 3:08 PM
Wizard marked 6 inline comments as done.

rename file

Wizard updated this revision to Diff 123091.Nov 15 2017, 3:10 PM
Wizard marked an inline comment as done.

rename again

File names look good now, thanks.

clang-tidy/google/AvoidThrowingObjCExceptionCheck.cpp
1 ↗(On Diff #123091)

Objc -> ObjC

Wizard marked an inline comment as done.Nov 15 2017, 3:13 PM
This revision was automatically updated to reflect the committed changes.