hokein added inline comments.
================ Comment at: clang-tidy/google/FunctionNamingCheck.cpp:57 + functionDecl( + isDefinition(), + unless(anyOf(isMain(), matchesName(validFunctionNameRegex(true)), ---------------- stephanemoore wrote: > hokein wrote: > > any reason why we restrict to definitions only? I think we can consider > > declarations too. > I restricted the check to function definitions because function declarations > are sometimes associated with functions outside the control of the author. I > have personally observed unfortunate cases where functions declared in the > iOS SDK had incorrect—or seemingly incorrect—availability attributes that > caused fatal dyld assertions during application startup. The check currently > intends to avoid flagging function declarations because of the rare > circumstances where an inflexible function declaration without a > corresponding function definition is required. > > I have added a comment explaining why only function definitions are flagged. > > I am still open to discussion though. Thanks for the explanations. I have a concern about the heuristic using here, it seems fragile -- if there is an inline function defined in a base header, the check will still give a warning to it if the source file `.m` #includes this header; it also limits the scope of the check, I think this check is flagged mostly on file-local functions (e.g. static functions, functions defined in anonymous namespace). Flagging on function declaration doesn't seem too problematic (we already have a similar check `objc-property-declaration` does the same thing) -- our internal review system shows check warnings on changed code, if user's code includes a common header which violates the check, warnings on the header will not be shown; for violations in iOS SDK, we can use some filtering matchers (`isExpansionInSystemHeader` maybe work) to ignore all functions from these files. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51575 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
