Bug 72483 - Add .dir-locals.el file for Emacs
Summary: Add .dir-locals.el file for Emacs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 02:21 PST by Andy Wingo
Modified: 2011-11-24 05:01 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.16 KB, patch)
2011-11-16 02:33 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (1.36 KB, patch)
2011-11-16 07:06 PST, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (1.27 KB, patch)
2011-11-16 09:42 PST, Andy Wingo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2011-11-16 02:21:30 PST
Emacs 23 and later support directory-local variables:

  http://www.gnu.org/s/libtool/manual/emacs/Directory-Variables.html

I occasionally see patches fail style checks because of indentation.  Adding a .dir-locals file to the repo would help avoid these, and as it's invisible to a normal `ls', it shouldn't be a burden on anyone.

Patch to be attached.
Comment 1 Andy Wingo 2011-11-16 02:33:47 PST
Created attachment 115353 [details]
Patch
Comment 2 Andy Wingo 2011-11-16 07:06:26 PST
Created attachment 115374 [details]
Patch
Comment 3 Andy Wingo 2011-11-16 07:07:13 PST
Updated with explicit blocks for c-mode and c++-mode, so that we really do override a user's default settings.
Comment 4 Martin Robinson 2011-11-16 08:40:51 PST
Comment on attachment 115353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115353&action=review

> .dir-locals.el:6
> +  (fill-column . 80)

The most common range I've heard is 120-180, but definitely not 80: https://lists.webkit.org/pipermail/webkit-dev/2009-September/009761.html

> .dir-locals.el:7
> +  (tab-width . 8)

Why not 4 here?
Comment 5 Andy Wingo 2011-11-16 08:51:37 PST
I must admit that I do not understand WebKit's predilection for long lines.  I assumed that people producing long lines were not using Emacs.  But I am fine putting in some other value.

Regarding tab widths, the docs say:

  Documentation:
  *Distance between tab stops (for display of tab characters), in columns.

That is the amount that TAB characters are interpreted as.  TAB characters are not usually present in WK, but where they are, as in Makefiles, they should probably be represented as 8 characters wide.  It is the default though, so it can probably be elided.

Do you have concrete recommendations?
Comment 6 Martin Robinson 2011-11-16 08:55:44 PST
(In reply to comment #5)
> I must admit that I do not understand WebKit's predilection for long lines.  I assumed that people producing long lines were not using Emacs.  But I am fine putting in some other value.

I would say either 180 or having no maximum.  At least in vim it can sometimes be annoying when the editor folds a pre-existing line when you modify it. Perosonally, I'm not a fan of long lines either, but the source contains a lot of code with very long lines.

> That is the amount that TAB characters are interpreted as.  TAB characters are not usually present in WK, but where they are, as in Makefiles, they should probably be represented as 8 characters wide.  It is the default though, so it can probably be elided.

You can probably just omit it. The only place we have tab characters is in our automake files.

I think it'd be best if an emacs user gives the final r+. :)
Comment 7 Xan Lopez 2011-11-16 09:31:42 PST
Comment on attachment 115374 [details]
Patch

I think it's best to just omit the maximum for fill-column, WebKit has lots of really crazy-long lines. Other than that, looks great!

Emacs is the default GNU editor.
Comment 8 Andy Wingo 2011-11-16 09:42:37 PST
Created attachment 115396 [details]
Patch
Comment 9 Andy Wingo 2011-11-16 09:43:18 PST
Comment on attachment 115396 [details]
Patch

Removed the fill-column and tab-width bits.  Xan, cq?
Comment 10 Xan Lopez 2011-11-21 03:37:20 PST
Comment on attachment 115396 [details]
Patch

Bam.
Comment 11 WebKit Review Bot 2011-11-21 04:48:30 PST
Comment on attachment 115396 [details]
Patch

Clearing flags on attachment: 115396

Committed r100902: <http://trac.webkit.org/changeset/100902>
Comment 12 WebKit Review Bot 2011-11-21 04:48:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Tommy Widenflycht 2011-11-23 05:44:36 PST
I'm not particularly happy with this since it interferes my my existing .dir-local.el that have something similar but also modifications to the compile command :/

But if it does something useful I guess I can live with it.
Comment 14 Martin Robinson 2011-11-23 08:06:02 PST
Perhaps there can be a .dir.locals.locals.el or something similar to allow extending the WebKit defaults.
Comment 15 Joshua Bell 2011-11-23 11:23:22 PST
(In reply to comment #14)
> Perhaps there can be a .dir.locals.locals.el or something similar to allow extending the WebKit defaults.

http://code.google.com/p/chromium/wiki/Emacs 

... recommends having .dir-locals.el specify (c-file-style . "WebKit") for c-mode/c++-mode so that additional customization can be done in your .emacs - this would at least be a standard way to allow users to make additional style changes.

While I'm skeptical that editor-specific style options should be baked into the tree like this, I'd personally be satisfied (for now) if the c-file-style entries were added.
Comment 16 Andy Wingo 2011-11-24 04:59:49 PST
(In reply to comment #15)
> http://code.google.com/p/chromium/wiki/Emacs 
> 
> ... recommends having .dir-locals.el specify (c-file-style . "WebKit")

That's an interesting page, thanks for the link.

There are definitely things you can do to improve webkit editing if you are willing to edit your .emacs, but my goal with this was to make things better for people that don't have .emacs customizations.

If you are editing your .emacs, something like:

(defun webkit-c++-mode ()
  "C++ mode with adjusted defaults for use with WebKit."
  (interactive)
  (c++-mode)
  (c-set-style "WebKit")
  (setq indent-tabs-mode t)
  (setq c-basic-offset 4))

(setq auto-mode-alist (cons '("/WebKit/.*\\.[ch]*$" . webkit-c++-mode)
      auto-mode-alist))
Comment 17 Andy Wingo 2011-11-24 05:01:06 PST
Properly indented ;)

(setq auto-mode-alist (cons '("/WebKit/.*\\.[ch]*$" . webkit-c++-mode)
                      auto-mode-alist))