Patch forthcoming
Created attachment 108024 [details] Add build phases that invoke a script that checks that Objective-C class names have appropriately-prefixed names
Reading the new shell script Tools/Scripts/check-for-inappropriate-objc-class-names as a diff against Tools/Scripts/check-for-weak-vtables-and-externals is a bit confusing. It might be worth attaching the shell script as a separate attachment so we can see it as a whole.
Created attachment 108029 [details] bad attachment This is the script added in attachment 108024 [details]
Created attachment 108030 [details] check-for-inappropriate-objc-class-names This is the script added in attachment 108024 [details].
Comment on attachment 108024 [details] Add build phases that invoke a script that checks that Objective-C class names have appropriately-prefixed names View in context: https://bugs.webkit.org/attachment.cgi?id=108024&action=review Thanks for putting up the attachment! Some drive-by comments about the script not meant to be a review. The script looks good to me, but I think someone who knows more about the `nm` output would be a better reviewer. > Tools/Scripts/check-for-inappropriate-objc-class-names:55 > +my $pattern = "^(" . join('|', @allowedPrefixes) . ")"; Maybe consider regex escaping the @allowedPrefixes values. This can be done with \Qvalue\E inside of perl regexes. But here the input is known to be simple strings so that might just be overkill. http://perldoc.perl.org/perlre.html#Quoting-metacharacters > Tools/Scripts/check-for-inappropriate-objc-class-names:59 > +if (!defined $executablePathAge || !defined $buildTimestampAge || $executablePathAge < $buildTimestampAge || $scriptAge < $buildTimestampAge) { This $scriptAge check looks useful. Maybe you could add that to check-for-weak-vtables-and-externals and other build scripts? > Tools/Scripts/check-for-inappropriate-objc-class-names:91 > + print "ERROR: Objective-C class names in $target must have the prefix \"" . $allowedPrefixes[0] . "\"."; This should end with a "\n".
Landed as <http://trac.webkit.org/r95655>. (In reply to comment #5) > (From update of attachment 108024 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108024&action=review > > Thanks for putting up the attachment! Some drive-by comments about the script not meant to be a review. The script looks good to me, but I think someone who knows more about the `nm` output would be a better reviewer. Thanks for the comments! > > > Tools/Scripts/check-for-inappropriate-objc-class-names:55 > > +my $pattern = "^(" . join('|', @allowedPrefixes) . ")"; > > Maybe consider regex escaping the @allowedPrefixes values. This can be done with \Qvalue\E inside of perl regexes. But here the input is known to be simple strings so that might just be overkill. I decided that it was overkill and would actually obfuscate the code. > This $scriptAge check looks useful. Maybe you could add that to check-for-weak-vtables-and-externals and other build scripts? Seems like a good idea. One other script already has this check. > > > Tools/Scripts/check-for-inappropriate-objc-class-names:91 > > + print "ERROR: Objective-C class names in $target must have the prefix \"" . $allowedPrefixes[0] . "\"."; > > This should end with a "\n". Done.