Page MenuHomePhabricator

Add WebKit brace style configuration option.
ClosedPublic

Authored by lifted on Aug 7 2015, 8:57 AM.

Details

Summary

Add brace style BS_WebKit as described on https://www.webkit.org/coding/coding-style.html:

  • Function definitions: place each brace on its own line.
  • Other braces: place the open brace on the line preceding the code block; place the close brace on its own line.

Set brace style used in getWebKitStyle() to the newly added BS_WebKit.

Diff Detail

Repository
rL LLVM

Event Timeline

lifted updated this revision to Diff 31518.Aug 7 2015, 8:57 AM
lifted retitled this revision from to Add Qt brace style configuration option..
lifted updated this object.
lifted added a reviewer: djasper.
lifted added subscribers: cfe-commits, klimek.
djasper accepted this revision.Aug 10 2015, 12:44 AM
djasper edited edge metadata.

Looks good. Do you have commit access?

include/clang/Format/Format.h
180 ↗(On Diff #31518)

Remove the second comma (and regenerate the docs)

This revision is now accepted and ready to land.Aug 10 2015, 12:44 AM

Hm, the referenced style guide doesn't say anything about namespaces. I also couldn't find anything in the qt source repo (just took a quick look though). Do you have an example of why Qt projects shouldn't just use BS_Linux?

Hm, the referenced style guide doesn't say anything about namespaces.

You're right. Futhermore, it's hard to find any namespaces in qt repos at all - Qt uses a macro for conditional compilation with namespaces.

I also couldn't find anything in the qt source repo (just took a quick look though).

Hm, I've just browsed through the qtcore and seen no evidence that qt uses attached braces everywhere but after classes and function. Qt use of style is inconsistent (sometimes even within a single file), and few examples I've looked at actually use something close to Mozilla style - they break on structs, enums, classes, and functions (http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qchar.h?id=13c16bbd06e0034210ba2164f1be208c49a6e3a7).

My bad, I read the Qt Brace style description and decided that it could fit my current project style (we break after functions, and some people also break after classes). But it turned out that Qt style is not as well defined as I thought, so I'm out of luck.

I believe it's better to abandon this patch.

I hope I will find some time to introduce extra configuration options to override pre-defined brace styles, as it was discussed long time ago.

Hooray, I've found the style I need - it's WebKit
https://www.webkit.org/coding/coding-style.html

So I believe there are other people waiting for the same style as me - https://code.google.com/p/chromium/issues/detail?id=454958

How do you feel about renaming BS_Qt -> BS_WebKit and reverting the changes in UnwrappedLineParser.cpp?

Webkit doesn't break for classes:

class MyClass {
    ...
};
lifted added a comment.EditedAug 10 2015, 5:41 AM

Webkit doesn't break for classes:

That's why we need to revert changes in UnwrappedLineParser.cpp.
The funny thing with WebKit style is that we don't need to add any logic to deal with it, any style different from existing ones is WebKit :)

It's actually ok for me to not break after classes - only couple of people in our project do this. I was just trying to find a style that is both ubiquitous and close enough to our code base.

Ah, ok. Interestingly, our current Webkit style uses BS_Stroustrup, which is incorrect for } else {. So yep, implementing BS_Webkit sounds like a win.

lifted updated this revision to Diff 31660.EditedAug 10 2015, 5:53 AM
lifted edited edge metadata.

Removed Qt brace style, added WebKit brace style.

Changed default brace style in the WebKit configuration to BS_WebKit.

lifted retitled this revision from Add Qt brace style configuration option. to Add WebKit brace style configuration option..Aug 10 2015, 5:55 AM
lifted updated this object.

Can you also fix the current code in getWebKitStyle?

lifted added a comment.EditedAug 10 2015, 6:01 AM

Can you also fix the current code in getWebKitStyle?

Yep, I've already done it. I'll include this information into the diff description.

lifted updated this object.Aug 10 2015, 6:04 AM
klimek accepted this revision.Aug 10 2015, 6:05 AM
klimek added a reviewer: klimek.

Ah, awesome, looks good then. Do you have commit access or need one of us to land it?

I had commit access earlier, but I haven't tried it for long time. I'll ask for help if there are any problems.

lifted closed this revision.Aug 10 2015, 6:44 AM
This revision was automatically updated to reflect the committed changes.