Bug 36573 - check-webkit-style should restore the original file name if it is used from Emacs's flymake so that it can get an correct header guard name
Summary: check-webkit-style should restore the original file name if it is used from E...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-24 19:24 PDT by Hayato Ito
Modified: 2010-03-29 23:08 PDT (History)
1 user (show)

See Also:


Attachments
fix-header-guard-name-for-flymake (2.84 KB, patch)
2010-03-24 22:31 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
fix-header-guard-name-for-flymake (3.35 KB, patch)
2010-03-25 01:51 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2010-03-24 19:24:07 PDT
If users run check-webkit-style against header file (*.h) from Emacs's flymake, the file name might contain '_flymake' suffix in the end of the basename. So we have to restore original file name to get a correct header guard name.

A similar change has already been committed to original cpplint.py. So I'd like to sync cpplint.py and cpp.py.
Comment 1 Hayato Ito 2010-03-24 22:31:10 PDT
Created attachment 51596 [details]
fix-header-guard-name-for-flymake
Comment 2 Shinichiro Hamaji 2010-03-24 23:21:56 PDT
Comment on attachment 51596 [details]
fix-header-guard-name-for-flymake

> -    emacs_flymake_suffix = '_flymake.cpp'
> -    if abs_filename.endswith(emacs_flymake_suffix):
> -        abs_filename = abs_filename[:-len(emacs_flymake_suffix)] + '.cpp'
> +    abs_filename = re.sub(r'_flymake\.cpp$', '.cpp', abs_filename)

Let me confirm, is this just a refactoring?

> +        # Special case for flymake
> +        error_collector = ErrorCollector(self.assert_)
> +        self.process_file_data('mydir/Foo_flymake.h', 'h', [], error_collector)
> +        self.assertEquals(
> +            1,
> +            error_collector.result_list().count(
> +                'No #ifndef header guard found, suggested CPP variable is: %s'
> +                '  [build/header_guard] [5]' % expected_guard),
> +            error_collector.result_list())
> +

I guess what we want to ensure is "#ifndef Foo_h_" won't be warned when it is put in "Foo_flymake.h" ? If so, please add a test case for this case.

By the way, I think putting your elisp to use check-webkit-style with flymake may help someone. Could you show it?
Comment 3 Hayato Ito 2010-03-25 01:51:57 PDT
Created attachment 51609 [details]
fix-header-guard-name-for-flymake
Comment 4 Hayato Ito 2010-03-25 01:56:41 PDT
Thank you for the review.

(In reply to comment #2)
> (From update of attachment 51596 [details])
> > -    emacs_flymake_suffix = '_flymake.cpp'
> > -    if abs_filename.endswith(emacs_flymake_suffix):
> > -        abs_filename = abs_filename[:-len(emacs_flymake_suffix)] + '.cpp'
> > +    abs_filename = re.sub(r'_flymake\.cpp$', '.cpp', abs_filename)
> 
> Let me confirm, is this just a refactoring?

Yes, it is just a refactoring.

> > +        # Special case for flymake
> > +        error_collector = ErrorCollector(self.assert_)
> > +        self.process_file_data('mydir/Foo_flymake.h', 'h', [], error_collector)
> > +        self.assertEquals(
> > +            1,
> > +            error_collector.result_list().count(
> > +                'No #ifndef header guard found, suggested CPP variable is: %s'
> > +                '  [build/header_guard] [5]' % expected_guard),
> > +            error_collector.result_list())
> > +
> 
> I guess what we want to ensure is "#ifndef Foo_h_" won't be warned when it is
> put in "Foo_flymake.h" ? If so, please add a test case for this case.

I've add a test case. Thank you!
 
> By the way, I think putting your elisp to use check-webkit-style with flymake
> may help someone. Could you show it?


Sure. It should be something like:

(require 'flymake)

(defun flymake-check-webkit-style-init ()
  (list "check-webkit-style" (list (flymake-init-create-temp-buffer-copy 'flymake-create-temp-inplace))))

(add-to-list 'flymake-allowed-file-name-masks
             '("WebKit/.*\\.cpp\\'" flymake-check-webkit-style-init))
(add-to-list 'flymake-allowed-file-name-masks
             '("WebKit/.*\\.h\\'" flymake-check-webkit-style-init))


After evaluating the above elisp, just doing M-x flymake.
Comment 5 Shinichiro Hamaji 2010-03-25 02:46:57 PDT
Comment on attachment 51609 [details]
fix-header-guard-name-for-flymake

Looks good! Thanks for adding the test and showing your elisp-fu.
Comment 6 WebKit Commit Bot 2010-03-29 23:08:41 PDT
Comment on attachment 51609 [details]
fix-header-guard-name-for-flymake

Clearing flags on attachment: 51609

Committed r56763: <http://trac.webkit.org/changeset/56763>
Comment 7 WebKit Commit Bot 2010-03-29 23:08:45 PDT
All reviewed patches have been landed.  Closing bug.