2009-12-18 Fumitoshi Ukai <ukai@chromium.org>
authorukai@chromium.org <ukai@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Dec 2009 17:47:21 +0000 (17:47 +0000)
committerukai@chromium.org <ukai@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Dec 2009 17:47:21 +0000 (17:47 +0000)
        Reviewed by David Levin.

        Check one space before end of line comments.
        https://bugs.webkit.org/show_bug.cgi?id=32597

        Fix to check one space before end of line comments in whitespace and build/header_guard.
        Also fix build/header_guard to use WebKit header guard defines.

        * Scripts/modules/cpp_style.py:
        * Scripts/modules/cpp_style_unittest.py:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@52318 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKitTools/ChangeLog
WebKitTools/Scripts/modules/cpp_style.py
WebKitTools/Scripts/modules/cpp_style_unittest.py

index 138f0ad..92f66ee 100644 (file)
@@ -1,3 +1,16 @@
+2009-12-18  Fumitoshi Ukai  <ukai@chromium.org>
+
+        Reviewed by David Levin.
+
+        Check one space before end of line comments.
+        https://bugs.webkit.org/show_bug.cgi?id=32597
+        
+        Fix to check one space before end of line comments in whitespace and build/header_guard.
+        Also fix build/header_guard to use WebKit header guard defines.
+
+        * Scripts/modules/cpp_style.py:
+        * Scripts/modules/cpp_style_unittest.py:
+
 2009-12-17  Sam Weinig  <sam@webkit.org>
 
         Reviewed by Mark Rowe.
index 3c67c25..4fba777 100644 (file)
@@ -69,7 +69,6 @@ _DEFAULT_OUTPUT_FORMAT = 'emacs'
 # the filter rules as specified by a --filter flag.
 _WEBKIT_FILTER_RULES = [
     '-build/endif_comment',
-    '-build/header_guard',
     '-build/include_what_you_use',  # <string> for std::string
     '-build/storage_class',  # const static
     '-legal/copyright',
@@ -87,7 +86,6 @@ _WEBKIT_FILTER_RULES = [
     '-runtime/threadsafe_fn',
     '-runtime/rtti',
     '-whitespace/blank_line',
-    '-whitespace/comments',
     '-whitespace/end_of_line',
     '-whitespace/labels',
     ]
@@ -874,8 +872,7 @@ def get_header_guard_cpp_variable(filename):
 
     """
 
-    fileinfo = FileInfo(filename)
-    return sub(r'[-./\s]', '_', fileinfo.repository_name()).upper() + '_'
+    return sub(r'[-.\s]', '_', os.path.basename(filename))
 
 
 def check_for_header_guard(filename, lines, error):
@@ -918,23 +915,14 @@ def check_for_header_guard(filename, lines, error):
               cppvar)
         return
 
-    # The guard should be PATH_FILE_H_, but we also allow PATH_FILE_H__
-    # for backward compatibility.
+    # The guard should be File_h.
     if ifndef != cppvar:
-        error_level = 0
-        if ifndef != cppvar + '_':
-            error_level = 5
-
-        error(filename, ifndef_line_number, 'build/header_guard', error_level,
+        error(filename, ifndef_line_number, 'build/header_guard', 5,
               '#ifndef header guard has wrong style, please use: %s' % cppvar)
 
-    if endif != ('#endif  // %s' % cppvar):
-        error_level = 0
-        if endif != ('#endif  // %s' % (cppvar + '_')):
-            error_level = 5
-
-        error(filename, endif_line_number, 'build/header_guard', error_level,
-              '#endif line should be "#endif  // %s"' % cppvar)
+    if endif != ('#endif // %s' % cppvar):
+        error(filename, endif_line_number, 'build/header_guard', 5,
+              '#endif line should be "#endif // %s"' % cppvar)
 
 
 def check_for_unicode_replacement_characters(filename, lines, error):
@@ -1524,14 +1512,14 @@ def check_spacing(filename, clean_lines, line_number, error):
         # Check if the // may be in quotes.  If so, ignore it
         # Comparisons made explicit for clarity -- pylint: disable-msg=C6403
         if (line.count('"', 0, comment_position) - line.count('\\"', 0, comment_position)) % 2 == 0:   # not in quotes
-            # Allow one space for new scopes, two spaces otherwise:
-            if (not match(r'^\s*{ //', line)
-                and ((comment_position >= 1
-                      and line[comment_position-1] not in string.whitespace)
+            # Allow one space before end of line comment.
+            if (not match(r'^\s*$', line[:comment_position])
+                and (comment_position >= 1
+                and ((line[comment_position - 1] not in string.whitespace)
                      or (comment_position >= 2
-                         and line[comment_position-2] not in string.whitespace))):
-                error(filename, line_number, 'whitespace/comments', 2,
-                      'At least two spaces is best between code and comments')
+                         and line[comment_position - 2] in string.whitespace)))):
+                error(filename, line_number, 'whitespace/comments', 5,
+                      'One space before end of line comments')
             # There should always be a space between the // and the comment
             commentend = comment_position + 2
             if commentend < len(line) and not line[commentend] == ' ':
index 80a9868..53e1205 100644 (file)
@@ -399,13 +399,13 @@ class CppStyleTest(CppStyleTestBase):
             '  [readability/casting] [4]')
         # Checks for false positives...
         self.assert_lint(
-            'int a = int();  // Constructor, o.k.',
+            'int a = int(); // Constructor, o.k.',
             '')
         self.assert_lint(
-            'X::X() : a(int()) {}  // default Constructor, o.k.',
+            'X::X() : a(int()) {} // default Constructor, o.k.',
             '')
         self.assert_lint(
-            'operator bool();  // Conversion operator, o.k.',
+            'operator bool(); // Conversion operator, o.k.',
             '')
 
     # The second parameter to a gMock method definition is a function signature
@@ -720,7 +720,7 @@ class CppStyleTest(CppStyleTestBase):
         # missing explicit, with distracting comment, is still bad
         self.assert_multi_line_lint(
             '''class Foo {
-                 Foo(int f);  // simpler than Foo(blargh, blarg)
+                 Foo(int f); // simpler than Foo(blargh, blarg)
                };''',
             'Single-argument constructors should be marked explicit.'
             '  [runtime/explicit] [5]')
@@ -1088,7 +1088,7 @@ class CppStyleTest(CppStyleTestBase):
                          '  [readability/check] [2]')
 
         self.assert_lint(
-            '    EXPECT_TRUE(42 < x)  // Random comment.',
+            '    EXPECT_TRUE(42 < x) // Random comment.',
             'Consider using EXPECT_LT instead of EXPECT_TRUE(a < b)'
             '  [readability/check] [2]')
         self.assert_lint(
@@ -1248,30 +1248,31 @@ class CppStyleTest(CppStyleTestBase):
                          '  [whitespace/operators] [3]')
         self.assert_lint('a<Foo*> t <<= *b/c;', 'Missing spaces around /'
                          '  [whitespace/operators] [3]')
-        self.assert_lint('a<Foo*> t <<= b/c; //Test', ['At least two spaces'
-                         ' is best between code and comments  [whitespace/'
-                         'comments] [2]', 'Should have a space between // '
-                         'and comment  [whitespace/comments] [4]', 'Missing'
+        self.assert_lint('a<Foo*> t <<= b/c; //Test', [
+                         'Should have a space between // and comment  '
+                         '[whitespace/comments] [4]', 'Missing'
                          ' spaces around /  [whitespace/operators] [3]'])
-        self.assert_lint('a<Foo*> t <<= b||c;  //Test', ['Should have a space'
-                         ' between // and comment  [whitespace/comments] [4]',
+        self.assert_lint('a<Foo*> t <<= b||c;  //Test', ['One space before end'
+                         ' of line comments  [whitespace/comments] [5]',
+                         'Should have a space between // and comment  '
+                         '[whitespace/comments] [4]',
                          'Missing spaces around ||  [whitespace/operators] [3]'])
-        self.assert_lint('a<Foo*> t <<= b&&c;  // Test', 'Missing spaces around'
+        self.assert_lint('a<Foo*> t <<= b&&c; // Test', 'Missing spaces around'
                          ' &&  [whitespace/operators] [3]')
-        self.assert_lint('a<Foo*> t <<= b&&&c;  // Test', 'Missing spaces around'
+        self.assert_lint('a<Foo*> t <<= b&&&c; // Test', 'Missing spaces around'
                          ' &&  [whitespace/operators] [3]')
-        self.assert_lint('a<Foo*> t <<= b&&*c;  // Test', 'Missing spaces around'
+        self.assert_lint('a<Foo*> t <<= b&&*c; // Test', 'Missing spaces around'
                          ' &&  [whitespace/operators] [3]')
-        self.assert_lint('a<Foo*> t <<= b && *c;  // Test', '')
-        self.assert_lint('a<Foo*> t <<= b && &c;  // Test', '')
+        self.assert_lint('a<Foo*> t <<= b && *c; // Test', '')
+        self.assert_lint('a<Foo*> t <<= b && &c; // Test', '')
         self.assert_lint('a<Foo*> t <<= b || &c;  /*Test', 'Complex multi-line '
                          '/*...*/-style comment found. Lint may give bogus '
                          'warnings.  Consider replacing these with //-style'
                          ' comments, with #if 0...#endif, or with more clearly'
                          ' structured multi-line comments.  [readability/multiline_comment] [5]')
         self.assert_lint('a<Foo&> t <<= &b | &c;', '')
-        self.assert_lint('a<Foo*> t <<= &b & &c;  // Test', '')
-        self.assert_lint('a<Foo*> t <<= *b / &c;  // Test', '')
+        self.assert_lint('a<Foo*> t <<= &b & &c; // Test', '')
+        self.assert_lint('a<Foo*> t <<= *b / &c; // Test', '')
         self.assert_lint('if (a=b == 1)', 'Missing spaces around =  [whitespace/operators] [4]')
         self.assert_lint('a = 1<<20', 'Missing spaces around <<  [whitespace/operators] [3]')
         self.assert_lint('if (a = b == 1)', '')
@@ -1305,7 +1306,7 @@ class CppStyleTest(CppStyleTestBase):
                          'For a static/global string constant, use a C style '
                          'string instead: "char foo[]".'
                          '  [runtime/string] [4]')
-        self.assert_lint('string kFoo = "hello";  // English',
+        self.assert_lint('string kFoo = "hello"; // English',
                          'For a static/global string constant, use a C style '
                          'string instead: "char kFoo[]".'
                          '  [runtime/string] [4]')
@@ -1364,25 +1365,27 @@ class CppStyleTest(CppStyleTestBase):
 
     def test_two_spaces_between_code_and_comments(self):
         self.assert_lint('} // namespace foo',
-                         'At least two spaces is best between code and comments'
-                         '  [whitespace/comments] [2]')
+                         '')
         self.assert_lint('}// namespace foo',
-                         'At least two spaces is best between code and comments'
-                         '  [whitespace/comments] [2]')
+                         'One space before end of line comments'
+                         '  [whitespace/comments] [5]')
         self.assert_lint('printf("foo"); // Outside quotes.',
-                         'At least two spaces is best between code and comments'
-                         '  [whitespace/comments] [2]')
-        self.assert_lint('int i = 0;  // Having two spaces is fine.', '')
-        self.assert_lint('int i = 0;   // Having three spaces is OK.', '')
+                         '')
+        self.assert_lint('int i = 0; // Having one space is fine.','')
+        self.assert_lint('int i = 0;  // Having two spaces is bad.',
+                         'One space before end of line comments'
+                         '  [whitespace/comments] [5]')
+        self.assert_lint('int i = 0;   // Having three spaces is bad.',
+                         'One space before end of line comments'
+                         '  [whitespace/comments] [5]')
         self.assert_lint('// Top level comment', '')
         self.assert_lint('    // Line starts with four spaces.', '')
         self.assert_lint('foo();\n'
                          '{ // A scope is opening.', '')
         self.assert_lint('    foo();\n'
                          '    { // An indented scope is opening.', '')
-        self.assert_lint('if (foo) { // not a pure scope; comment is too close!',
-                         'At least two spaces is best between code and comments'
-                         '  [whitespace/comments] [2]')
+        self.assert_lint('if (foo) { // not a pure scope',
+                         '')
         self.assert_lint('printf("// In quotes.")', '')
         self.assert_lint('printf("\\"%s // In quotes.")', '')
         self.assert_lint('printf("%s", "// In quotes.")', '')
@@ -1561,7 +1564,7 @@ class CppStyleTest(CppStyleTestBase):
     def test_tab(self):
         self.assert_lint('\tint a;',
                          'Tab found; better to use spaces  [whitespace/tab] [1]')
-        self.assert_lint('int a = 5;\t\t// set a to 5',
+        self.assert_lint('int a = 5;\t// set a to 5',
                          'Tab found; better to use spaces  [whitespace/tab] [1]')
 
     def test_parse_arguments(self):
@@ -1730,7 +1733,7 @@ class CppStyleTest(CppStyleTestBase):
                          '  [build/forward_decl] [5]')
 
     def test_build_header_guard(self):
-        file_path = 'mydir/foo.h'
+        file_path = 'mydir/Foo.h'
 
         # We can't rely on our internal stuff to get a sane path on the open source
         # side of things, so just parse out the suggested header guard. This
@@ -1740,7 +1743,7 @@ class CppStyleTest(CppStyleTestBase):
         cpp_style.process_file_data(file_path, 'h', [], error_collector)
         expected_guard = ''
         matcher = re.compile(
-            'No \#ifndef header guard found\, suggested CPP variable is\: ([A-Z_0-9]+) ')
+            'No \#ifndef header guard found\, suggested CPP variable is\: ([A-Za-z_0-9]+) ')
         for error in error_collector.result_list():
             matches = matcher.match(error)
             if matches:
@@ -1794,7 +1797,7 @@ class CppStyleTest(CppStyleTestBase):
         self.assertEquals(
             1,
             error_collector.result_list().count(
-                '#endif line should be "#endif  // %s"'
+                '#endif line should be "#endif // %s"'
                 '  [build/header_guard] [5]' % expected_guard),
             error_collector.result_list())
 
@@ -1808,7 +1811,7 @@ class CppStyleTest(CppStyleTestBase):
         self.assertEquals(
             1,
             error_collector.result_list().count(
-                '#endif line should be "#endif  // %s"'
+                '#endif line should be "#endif // %s"'
                 '  [build/header_guard] [5]' % expected_guard),
             error_collector.result_list())
 
@@ -1822,7 +1825,7 @@ class CppStyleTest(CppStyleTestBase):
         self.assertEquals(
             1,
             error_collector.result_list().count(
-                '#endif line should be "#endif  // %s"'
+                '#endif line should be "#endif // %s"'
                 '  [build/header_guard] [5]' % expected_guard),
             error_collector.result_list())
 
@@ -1831,42 +1834,12 @@ class CppStyleTest(CppStyleTestBase):
         cpp_style.process_file_data(file_path, 'h',
                                     ['#ifndef %s' % expected_guard,
                                      '#define %s' % expected_guard,
-                                     '#endif  // %s' % expected_guard],
-                                    error_collector)
-        for line in error_collector.result_list():
-            if line.find('build/header_guard') != -1:
-                self.fail('Unexpected error: %s' % line)
-
-        # No header guard errors for old-style guard
-        error_collector = ErrorCollector(self.assert_)
-        cpp_style.process_file_data(file_path, 'h',
-                                    ['#ifndef %s_' % expected_guard,
-                                     '#define %s_' % expected_guard,
-                                     '#endif  // %s_' % expected_guard],
+                                     '#endif // %s' % expected_guard],
                                     error_collector)
         for line in error_collector.result_list():
             if line.find('build/header_guard') != -1:
                 self.fail('Unexpected error: %s' % line)
 
-        old_verbose_level = cpp_style._cpp_style_state.verbose_level
-        try:
-            cpp_style._cpp_style_state.verbose_level = 0
-            # Warn on old-style guard if verbosity is 0.
-            error_collector = ErrorCollector(self.assert_)
-            cpp_style.process_file_data(file_path, 'h',
-                                        ['#ifndef %s_' % expected_guard,
-                                         '#define %s_' % expected_guard,
-                                         '#endif  // %s_' % expected_guard],
-                                        error_collector)
-            self.assertEquals(
-                1,
-                error_collector.result_list().count(
-                    '#ifndef header guard has wrong style, please use: %s'
-                    '  [build/header_guard] [0]' % expected_guard),
-                error_collector.result_list())
-        finally:
-            cpp_style._cpp_style_state.verbose_level = old_verbose_level
-
         # Completely incorrect header guard
         error_collector = ErrorCollector(self.assert_)
         cpp_style.process_file_data(file_path, 'h',
@@ -1883,7 +1856,7 @@ class CppStyleTest(CppStyleTestBase):
         self.assertEquals(
             1,
             error_collector.result_list().count(
-                '#endif line should be "#endif  // %s"'
+                '#endif line should be "#endif // %s"'
                 '  [build/header_guard] [5]' % expected_guard),
             error_collector.result_list())
 
@@ -2912,7 +2885,7 @@ class WebKitStyleTest(CppStyleTestBase):
             'foo.cpp')
         self.assert_multi_line_lint(
             'namespace WebCore {\n\n'
-            'const char* foo(void* a = ";",  // ;\n'
+            'const char* foo(void* a = ";", // ;\n'
             '    void* b);\n'
             '    void* p;\n'
             '}\n',
@@ -2921,7 +2894,7 @@ class WebKitStyleTest(CppStyleTestBase):
         self.assert_multi_line_lint(
             'namespace WebCore {\n\n'
             'const char* foo[] = {\n'
-            '    "void* b);",  // ;\n'
+            '    "void* b);", // ;\n'
             '    "asfdf",\n'
             '    }\n'
             '    void* p;\n'
@@ -2931,7 +2904,7 @@ class WebKitStyleTest(CppStyleTestBase):
         self.assert_multi_line_lint(
             'namespace WebCore {\n\n'
             'const char* foo[] = {\n'
-            '    "void* b);",  // }\n'
+            '    "void* b);", // }\n'
             '    "asfdf",\n'
             '    }\n'
             '}\n',
@@ -2941,7 +2914,7 @@ class WebKitStyleTest(CppStyleTestBase):
             '    namespace WebCore {\n\n'
             '    void Document::Foo()\n'
             '    {\n'
-            'start:  // infinite loops are fun!\n'
+            'start: // infinite loops are fun!\n'
             '        goto start;\n'
             '    }',
             'namespace should never be indented.  [whitespace/indent] [4]',
@@ -3495,7 +3468,7 @@ class WebKitStyleTest(CppStyleTestBase):
             '  [readability/null] [4]',
             'foo.cpp')
         self.assert_lint(
-            '"A string with NULL"  // and a comment with NULL is tricky to flag correctly in cpp_style.',
+            '"A string with NULL" // and a comment with NULL is tricky to flag correctly in cpp_style.',
             'Use 0 instead of NULL.'
             '  [readability/null] [4]',
             'foo.cpp')
@@ -3712,6 +3685,15 @@ class WebKitStyleTest(CppStyleTestBase):
         self.assert_lint('typedef VectorType::const_iterator const_iterator;', '')
 
 
+    def test_comments(self):
+        # A comment at the beginning of a line is ok.
+        self.assert_lint('// comment', '')
+        self.assert_lint('    // comment', '')
+
+        self.assert_lint('}  // namespace WebCore',
+                         'One space before end of line comments'
+                         '  [whitespace/comments] [5]')
+
     def test_other(self):
         # FIXME: Implement this.
         pass