[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] [clang-format] Added: FormatStyle::IndentNamespaces to indent the content of namespaces
From: Daniel Jasper <djasper () google ! com>
Date: 2013-07-31 23:24:03
Message-ID: CAK_tg0wrNBVjaccz86xoUPNMC+YREFTGPgYaMMr+K1w=RX8udQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
I have extended your patch and submitted it as r187540.
Cheers,
Daniel
On Mon, Jul 29, 2013 at 3:08 AM, Daniel Jasper <djasper@google.com> wrote:
>
>
>
> On Mon, Jul 29, 2013 at 12:00 PM, Curdeius Curdeius <curdeius@gmail.com>wrote:
>
>> Hi,
>> thanks for your comments.
>> I've fixed the patch.
>> File formatting should be ok now.
>> I've changed the name to IndentInNamespaces as suggested.
>>
>
> Thank you. Do you have commit access or should I commit this for you?
>
> What about the indenting of inner namespaces: take a look for instance at
>> WebKit coding style about namespaces:
>> http://www.webkit.org/coding/coding-style.html#indentation-namespace.
>> It's not exactly what I proposed, but it's close still.
>>
>
> I will implement what is required for WebKit. Lets see how we go from
> there.
>
> Cheers,
> Daniel
>
> Cheers,
>> Marek Kurdej
>>
>>
>> 2013/7/29 Daniel Jasper <djasper@google.com>
>>
>>> Please attach patches as plain text, not as zip-files. Or alternatively,
>>> use phabricator (http://llvm-reviews.chandlerc.com/).
>>>
>>> Comments inline.
>>>
>>> + /// \brief Indent the content of namespaces by one level.
>>>> + ///
>>>> + /// When false, use the same indentation level as outside block
>>>> (file or namespace).
>>>>
>>>
>>> Please adhere to the 80 column limit. Or ideally, just use clang-format
>>> ;-).
>>>
>>>
>>>> + bool IndentNamespaces;
>>>>
>>>
>>> IndentInNamespaces might be slightly more precise, but I am not sure.
>>>
>>>
>>>> - parseBlock(/*MustBeDeclaration=*/true, 0);
>>>> + parseBlock(/*MustBeDeclaration=*/true,
>>>> /*AddLevels=*/Style.IndentNamespaces);
>>>>
>>>
>>> Please adhere to the 80 column limit.
>>>
>>>
>>>> + FormatStyle getLLVMStyleWithIndentNamespaces(bool IndentNamespaces =
>>>> true) {
>>>> + FormatStyle Style = getLLVMStyle();
>>>> + Style.IndentNamespaces = IndentNamespaces;
>>>> + return Style;
>>>> + }
>>>> +
>>>> + FormatStyle getGoogleStyleWithIndentNamespaces(bool IndentNamespaces
>>>> = true) {
>>>> + FormatStyle Style = getGoogleStyle();
>>>> + Style.IndentNamespaces = IndentNamespaces;
>>>> + return Style;
>>>> + }
>>>>
>>>
>>> I don't think it is worth adding these methods. Just define a local
>>> style in your test:
>>>
>>> TEST_F(..., ...) {
>>> FormatStyle Style = getLLVMStyle();
>>> Style.IndentNamespace = true;
>>> ...
>>> }
>>>
>>>
>>>> On Fri, Jul 26, 2013 at 2:17 PM, Curdeius Curdeius <curdeius@gmail.com>wrote:
>>>> Hi,
>>>> Clang-format was lacking an option to add indentation within namespaces.
>>>> I've created a patch that adds this option to
>>>> clang::Format::FormatStyle.
>>>> There is also a basic test.
>>>> You'll find it in the attachment.
>>>> I think it will be nice to have a possibility to indent only the
>>>> content of namespaces, but excluding the inner namespaces, i.e. to have
>>>> something like:
>>>>
>>>> class Indented1;
>>>> namespace inner1 {
>>>> class Indented2;
>>>> } // namespace inner1
>>>> namespace inner2 {
>>>> } // namespace inner2
>>>> } // namespace outer
>>>> instead of:
>>>>
>>>> class Indented1;
>>>> namespace inner1 {
>>>> class Indented2;
>>>> } // namespace inner1
>>>> namespace inner2 {
>>>> } // namespace inner2
>>>> } // namespace outer
>>>>
>>>
>>> Really? I have never seen the latter before and it would confuse me
>>> quite a bit.
>>>
>>> Cheers,
>>> Daniel
>>>
>>>
>>
>
[Attachment #5 (text/html)]
<div dir="ltr">I have extended your patch and submitted it as \
r187540.<div><br></div><div>Cheers,<br>Daniel</div></div><div \
class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 29, 2013 at 3:08 AM, \
Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" \
target="_blank">djasper@google.com</a>></span> wrote:<br> <blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div \
class="gmail_quote"><div class="im">On Mon, Jul 29, 2013 at 12:00 PM, Curdeius \
Curdeius <span dir="ltr"><<a href="mailto:curdeius@gmail.com" \
target="_blank">curdeius@gmail.com</a>></span> wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr">Hi,<div class="im"><div>thanks for your \
comments.</div><div>I've fixed the patch.</div><div>File formatting should be ok \
now.</div>
<div>I've changed the name to IndentInNamespaces as \
suggested.</div></div></div></blockquote><div><br></div><div>Thank you. Do you have \
commit access or should I commit this for you? </div><div class="im"><div><br></div> \
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <div dir="ltr"><div>
What about the indenting of inner namespaces: take a look for instance at WebKit \
coding style about namespaces: <a \
href="http://www.webkit.org/coding/coding-style.html#indentation-namespace" \
target="_blank">http://www.webkit.org/coding/coding-style.html#indentation-namespace</a>.</div>
<div>It's not exactly what I proposed, but it's close \
still.</div></div></blockquote><div><br></div></div><div>I will implement what is \
required for WebKit. Lets see how we go from there.</div><div><br>Cheers,</div> <div>
Daniel</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div \
dir="ltr"><div>Cheers,</div><div>Marek Kurdej<br></div><div><div> <div \
class="gmail_extra">
<br><br><div class="gmail_quote">2013/7/29 Daniel Jasper <span dir="ltr"><<a \
href="mailto:djasper@google.com" \
target="_blank">djasper@google.com</a>></span><br><blockquote class="gmail_quote" \
style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>Please attach patches as plain text, not as zip-files. Or \
alternatively, use phabricator (<a href="http://llvm-reviews.chandlerc.com/" \
target="_blank">http://llvm-reviews.chandlerc.com/</a>).</div> <div>
<br></div><div>Comments inline.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ /// \brief Indent the content of namespaces by one level.<br>+ ///<br>+ /// \
When false, use the same indentation level as outside block (file or \
namespace).<br></blockquote><div><br></div><div>Please adhere to the 80 column limit. \
Or ideally, just use clang-format ;-).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ \
bool IndentNamespaces;<br></blockquote><div>
<br></div><div>IndentInNamespaces might be slightly more precise, but I am not \
sure.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- parseBlock(/*MustBeDeclaration=*/true, 0);<br>+ \
parseBlock(/*MustBeDeclaration=*/true, \
/*AddLevels=*/Style.IndentNamespaces);<br></blockquote><div><br></div><div>Please \
adhere to the 80 column limit.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ \
FormatStyle getLLVMStyleWithIndentNamespaces(bool IndentNamespaces = true) {<br>
+ FormatStyle Style = getLLVMStyle();<br>+ Style.IndentNamespaces = \
IndentNamespaces;<br> + return Style;<br>+ }<br>+ <br>+ FormatStyle \
getGoogleStyleWithIndentNamespaces(bool IndentNamespaces = true) {<br>+ \
FormatStyle Style = getGoogleStyle();<br>+ Style.IndentNamespaces = \
IndentNamespaces;<br>+ return Style;<br>
+ }<br></blockquote><div><br></div><div>I don't think it is worth adding these \
methods. Just define a local style in your test:</div><div><br></div><div>TEST_F(..., \
...) {</div><div> FormatStyle Style = getLLVMStyle();</div>
<div> Style.IndentNamespace = true;</div><div> \
...</div><div>}</div><div><br></div><blockquote class="gmail_quote" style="margin:0px \
0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>
<br>On Fri, Jul 26, 2013 at 2:17 PM, Curdeius Curdeius <span dir="ltr"><<a \
href="mailto:curdeius@gmail.com" target="_blank">curdeius@gmail.com</a>></span> \
wrote:<br>Hi,<br>Clang-format was lacking an option to add indentation within \
namespaces.<br>
I've created a patch that adds this option to \
clang::Format::FormatStyle.<br>There is also a basic test.<br>You'll find it in \
the attachment.<br>I think it will be nice to have a possibility to indent only the \
content of namespaces, but excluding the inner namespaces, i.e. to have something \
like:<br>
<br></div><div> class Indented1;<br>namespace inner1 {<br>
class Indented2;<br>} // namespace inner1<br>namespace inner2 {<br>} // namespace \
inner2<br>} // namespace outer<br>instead of: <br> <br> class Indented1;<br> \
namespace inner1 {<br> class Indented2;<br> } // namespace inner1<br>
namespace inner2 {<br> } // namespace inner2<br>} // namespace \
outer<br></div></blockquote><div><br></div><div>Really? I have never seen the latter \
before and it would confuse me quite a bit.</div><div><br></div><div>
Cheers,<br>
Daniel</div><div class="gmail_extra"><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic