benhamilton requested changes to this revision. benhamilton added a comment. This revision now requires changes to proceed.
Almost there. ================ Comment at: clang-tidy/google/AvoidThrowingObjcExceptionCheck.cpp:22 +void AvoidThrowingObjcExceptionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(objcThrowStmt().bind("throwStmt"), this); +} ---------------- 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 ================ Comment at: clang-tidy/google/AvoidThrowingObjcExceptionCheck.cpp:30 + diag(MatchedStmt->getThrowLoc(), + "avoid using @throw to handle Objective-C exceptions"); +} ---------------- Suggested wording: "Pass in NSError ** instead of using @throw to indicate Objective-C errors" ================ Comment at: clang-tidy/google/AvoidThrowingObjcExceptionCheck.h:1 +//===--- AvoidThrowingObjcExceptionCheck.h - clang-tidy----------*- C++ -*-===// +// ---------------- Naming nit-pick: We are currently using `ObjC`, not `Objc`. ================ Comment at: clang-tidy/google/AvoidThrowingObjcExceptionCheck.h:21-22 +/// The check is to find usage of @throw invocation in Objective-C code. +/// We should avoid using @throw for Objective-C exceptions according to +/// Google Objective-C Style Guide. +/// ---------------- Grammar nit: according to Google Objective-C Style Guide -> according to the Google Objective-C Style Guide ================ Comment at: clang-tidy/google/AvoidThrowingObjcExceptionCheck.h:26 +/// http://clang.llvm.org/extra/clang-tidy/checks/google-objc-avoid-throwing-exception.html +class AvoidThrowingObjcExceptionCheck : public ClangTidyCheck { + public: ---------------- `Objc` -> `ObjC` ================ Comment at: docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst:6-7 + +This check finds @throw usages in Objective-C files. According to Google's Objective-C +Guide, we should not use @throw to handle exceptions. + ---------------- How about: This check finds @throw usages in Objective-C files. [For the same reasons as the Google C++ style guide](https://google.github.io/styleguide/cppguide.html#Exceptions), we [prefer not throwing exceptions from Objective-C code](http://google.github.io/styleguide/objcguide.html#avoid-throwing-exceptions). Instead, prefer passing in `NSError **` and return `BOOL` to indicate success or failure. A counterexample: ```lang=objectivec - (void)readFile { if ([self isError]) { @throw [NSException exceptionWithName:...]; } } ``` Instead, returning an error via `NSError **` is preferred: ```lang=objectivec - (BOOL)readFileWithError:(NSError **)error { if ([self isError]) { *error = [NSError errorWithDomain:...]; return NO; } return YES; } ``` ================ Comment at: docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst:10 +The corresponding style guide rule: +http://go/objc-style#Avoid_Throwing_Exceptions ---------------- hokein wrote: > Use the public link: > http://google.github.io/styleguide/objcguide.html#avoid-throwing-exceptions +1 https://reviews.llvm.org/D40058 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits