Page MenuHomePhabricator

clang-format: Add CompactNamespaces option
ClosedPublic

Authored by Typz on Apr 25 2017, 1:35 AM.

Details

Summary

Add CompactNamespaces option, to pack namespace declarations on the
same line (somewhat similar to C++17 nested namespace definition).

With this option, consecutive namespace declarations are kept on the
same line:

namespace foo { namespace bar {
    ...
}} // namespace foo::bar

Diff Detail

Repository
rL LLVM

Event Timeline

Typz created this revision.Apr 25 2017, 1:35 AM
djasper edited edge metadata.Apr 26 2017, 5:36 AM

For this and the other patches, could you please read: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
And make a statement as to whether these options qualify?

Typz added a comment.Apr 26 2017, 6:31 AM

This option is used in multiple projects with public coding styles: OpenCV (http://code.opencv.org/projects/opencv/wiki/Coding_Style_Guide), Boost (http://www.boost.org/development/requirements.html)
And I am indeed willing to maintain the patch.

kimgr added a subscriber: kimgr.May 16 2017, 6:58 AM

We have a large closed-source codebase with this style, and would welcome clang-format support.

djasper added inline comments.May 17 2017, 6:01 AM
include/clang/Format/Format.h
153 ↗(On Diff #96521)

While I am not entirely opposed to this feature, I think it should be a separate patch.

358 ↗(On Diff #96521)

This is not bin packing at all. Maybe CompactNamespaces? Or SingleLineNamespaces?

Also, could you clarify what happens if the namespaces don't fit within the column limit (both in the comment describing the flag and by adding tests for it)?

Typz added inline comments.May 17 2017, 8:04 AM
include/clang/Format/Format.h
153 ↗(On Diff #96521)

I totally agree, which is why I created 2 commits; however, since they modify the same code (and I am new to Phabricator) I could not find a way to upload them separately.

Is there a way? Or should I upload one, and upload the next one once the first one has been accepted?

Typz updated this revision to Diff 99421.May 18 2017, 4:51 AM

clang-format: Add CompactNamespaces option

  • Change option name to CompactNamespaces
  • Clarify & test behavior when wrapping is needed
  • Separate from the 'remove semicolon' patch
Typz retitled this revision from [clang-format] Add BinPackNamespaces option to clang-format: Add CompactNamespaces option.May 18 2017, 4:52 AM
Typz edited the summary of this revision. (Show Details)
Typz added a project: Restricted Project.
Typz edited the summary of this revision. (Show Details)
Typz marked 2 inline comments as done.
Typz added inline comments.
include/clang/Format/Format.h
153 ↗(On Diff #96521)
358 ↗(On Diff #96521)

This is not binpacking, but has a similar effect and made the option name somewhat consistent with the other binpack options :-)
I'll change to CompactNamespaces then.

Typz marked 4 inline comments as done.May 18 2017, 4:54 AM
djasper added inline comments.May 18 2017, 11:54 PM
include/clang/Format/Format.h
358 ↗(On Diff #96521)

How does this option interact with NamespaceIndentation. Do all the values you can set there make sense and work as expected?

I am wondering whether we should generally rename this to NamespaceStyle and make it an enum. That way, we can start to also support C++17 namespace, but people that don't want to use C++17 yet, can still use this style of compact namespace.

769 ↗(On Diff #99421)

What happened here?

lib/Format/NamespaceEndCommentsFixer.cpp
124 ↗(On Diff #99421)

no space after "*"..

127 ↗(On Diff #99421)

s/NULL/nullptr/

172 ↗(On Diff #99421)

Do we need any of this if CompactNamespaces if false? Maybe we should surround the entire thing with an

if (Style.CompactNamespaces) {
lib/Format/UnwrappedLineFormatter.cpp
130 ↗(On Diff #99421)

You always seem to call this function with Line->First. Maybe just call it on an AnnotatedLine?

134 ↗(On Diff #99421)

just

return NamespaceTok && NamespaceTok->is(tok::kw_namespace);
139 ↗(On Diff #99421)

s/line/Line/

221 ↗(On Diff #99421)

This looks like you wanted to write a for loop.

223 ↗(On Diff #99421)

Indentation

unittests/Format/FormatTest.cpp
1310 ↗(On Diff #99421)

There need to be more tests. Specifically:

// Input is already in the desired format
namespace A { namespace B { .. }} // namespace A::B

// Input is contracted, but wrong comment
namespace A { namespace B {..}} // namespace A

// Input is partially contracted
namespace A { namespace B { namespace C {
}} // namespace B::C
} // namespace A
Typz updated this revision to Diff 99575.May 19 2017, 9:22 AM
Typz marked 11 inline comments as done.
Typz edited the summary of this revision. (Show Details)

update for review comments

Typz added inline comments.May 19 2017, 9:24 AM
include/clang/Format/Format.h
358 ↗(On Diff #96521)

How does this option interact with NamespaceIndentation. Do all the values you can set there make sense and work as expected?

NamespaceIndentation is not affected, indent is done as before (e.g. just "counting" the imbricated namespaces).

In 'NI_All' we may want to reduce the indent when multiple namespaces are declared in the same line, but this would become inconsistent if the beginning and end of all namespaces do not match:

namepace A { namespace B {
    int i;
} // namespace B
  int i;
} // namespace A

So I think reducing indent in that case should be (if ever needed) another value in NamespaceIndentation...

I am wondering whether we should generally rename this to NamespaceStyle and make it an enum. That way, we can start to also support C++17 namespace, but people that don't want to use C++17 yet, can still use this style of compact namespace.

As for C++17, I am not sure we need another option: just having CompactNamespaces=true and Standard=C++17 would use the "real" C++17 mode. That said converting to C++17 namespace blocks is slightly more restrictive, as it will require that both the beginning and end of the inner & outer blocks to match...

I will keep the boolean flag for now, just let me know if you prefer to have the enum in case other modes are needed and I will update the patch.

lib/Format/UnwrappedLineFormatter.cpp
139 ↗(On Diff #99421)

these methods (isEndOfNamespace, ...) are somewhat duplicated in UnwrapperLinesFormatter and NamespaceEndComments (not really, but very similar) : should I factorize these into helper methods of FormatToken?

djasper added inline comments.May 22 2017, 2:07 AM
include/clang/Format/Format.h
358 ↗(On Diff #96521)

Yeah, this is probably something nobody will ever want:

namepace A { namespace B {
    int i;
} // namespace B
  int i;
} // namespace A

And you have the same problem for NI_Inner as soon as you have more than two levels of namespace.

I see two ways forward:

  1. Make "compacted" namespaces always add at most one level of indentation.
  2. Assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

The problem I have with #1 is that it increases the complexity quite a bit and the behavior is even difficult to predict to users. Remove a comment somewhere might enable clang-format to make two namespaces "compact" and then remove indentation throughout the file.

So I would lean more towards solution #2. The name NamespaceIndentation might then be a bit confusing, but not sure whether it's worth changing it. And of course I don't know whether some people would want indentation in compacted namespaces.

What do you think?

Typz added inline comments.May 22 2017, 2:43 AM
include/clang/Format/Format.h
358 ↗(On Diff #96521)

I think current behavior is at least consistent (and simple to implement), and I agree that #2 would be the way to go to implement special nanespace indent rules for compacted namespaces.

Personnally, I don't need namespace indentation (we use NI_None), but i can add try to add an extra style NI_Compact to indent one level when in any number of namespaces (e.g. indentLevel = namespaceDepth>0).

That should probably be a separate patch however?

What I mean is that you should remove the CompactNamespace option and instead let this be configured by an additional enum value in NamespaceIndentation.

Typz added a comment.May 22 2017, 5:49 AM

Merging the 2 options is definitely a "safe" option, as it prevents ensures only the most obvious behavior is accessible.

However, it has significant (IMO) drawbacks:

  • "Compact" is a not an namespace indentation type, this will make the option quite confusing
  • If indentation inside compact namespaces is needed, it cannot easily be added: we would need an extra mode NI_CompactWithIndent

All in all, I think I prefer the current behavior of the patch (a separate CompactNamespace options, with consistent [if not useful] indentation settings); but it is your call, just let me know how to proceed.

Typz updated this revision to Diff 99763.May 22 2017, 7:43 AM

Remove dependency on D33314

That's what I meant by "The name NamespaceIndentation might then be a bit confusing, but not sure whether it's worth changing it.".

I am honestly not sure. Let's get a third opinion.

If we add this additional option, I think we need to fix the behavior and make what people expect:

With NI_None:

namespace A { namespace B {
int i;
namespace C {
int j;
} // namespace C
}} // namespace A::B

With NI_Inner:

namespace A { namespace B {
int i;
namespace C {
  int j;
} // namespace C
}} // namespace A::B

With NI_All:

namespace A { namespace B {
  int i;
  namespace C {
    int j;
  } // namespace C
}} // namespace A::B
krasimir edited edge metadata.May 23 2017, 4:39 AM

This change should also adapt NamespaceEndCommentFixer to respect the new option and not introduce/remove/change the comments unexpectedly.

Typz added a comment.May 23 2017, 4:40 AM

Just to be clear: as it is, the patch does not implement what you describe, both on the 'indent' side (as expected, we are discussing it), but also on the "merging" of braces side. Consecutive namespace opening are merged on one side; and consecutive namespace closing are merged on the other side; but there is no relation between the two.

What this means is that at the moment your exemple would be formatted (without indent) like this:

namespace A { namespace B {
int i;
namespace C {
int j;
}}} // namespace A::B::C
Typz added a comment.May 23 2017, 4:43 AM

This change should also adapt NamespaceEndCommentFixer to respect the new option and not introduce/remove/change the comments unexpectedly.

This should be handled already (pending further changes to the patch, obviously); did you see (or think of) any specific broken or dangerous case?

In any case, adding a namespace end comment to a line closing multiple namespaces is super confusing for me: what does the comment refer to: the inner one, the outer one, or both?
}} // namespace A::B

Typz added a comment.EditedMay 23 2017, 5:32 AM

In any case, adding a namespace end comment to a line closing multiple namespaces is super confusing for me: what does the comment refer to: the inner one, the outer one, or both?
}} // namespace A::B

it refers to both, similar to C++17' _nested namespace_ definition:

namespace A::B {
}} // namespace A::B

But the whole problem is indeed that we are trying to do something similar, but in a more generic situations: leading to either a complicated implementation to merge only in the same conditions as C++17 (e.g. both namespaces begin and end at the same place) or have to handle somewhat complicated scenarios, with the blocks being partially merged (as leading to complications in indentation...)

klimek edited edge metadata.May 30 2017, 4:38 AM

I'm less concerned about everything suddenly re-indenting when you change code - if you use any kind of namespace indentation, that's what will happen now and then (and is why many style guides do not indent in namespaces).

For what it's worth, I'd

  1. vote for only merging in cases where both start and end can be merged
  2. vote for merge namespaces counting as a single namespace decl
Typz added a comment.May 31 2017, 2:54 AM

That will be slightly more complicated to check, esp. we will need to keep track of the matching-closing-brace (currently only the matching-opening-brace) is stored.
But I can try to update the patch in that direction, if that is the consensus.

Typz added a comment.Jun 6 2017, 5:05 AM

So how do I proceed?

  1. Keep the CompactNamespace option, and make "compacted" namespaces always add at most one level of indentation
  2. Or assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

And should we limit 'compacting' to namespace which start and end at consecutive lines [e.g. exactly the same conditions as C++17 nested namespaces], or is the current implementation enough [merge all adjacent beginning and all adjacent endings] ?

In D32480#773807, @Typz wrote:

So how do I proceed?

  1. Keep the CompactNamespace option, and make "compacted" namespaces always add at most one level of indentation
  2. Or assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

I'd vote for (1).

And should we limit 'compacting' to namespace which start and end at consecutive lines [e.g. exactly the same conditions as C++17 nested namespaces], or is the current implementation enough [merge all adjacent beginning and all adjacent endings] ?

I'd vote for the former.

Ok. Works for me.

Typz updated this revision to Diff 102346.Jun 13 2017, 9:04 AM
  • make "compacted" namespaces always add at most one level of indentation
  • compact only namespaces which both start and end on consecutive lines
Typz marked 3 inline comments as done.Jun 13 2017, 9:05 AM

Generally LG from my side.

unittests/Format/FormatTest.cpp
1363–1381 ↗(On Diff #102346)

These tests become more readable if you set up a style with a smaller column limit.

djasper accepted this revision.Jun 14 2017, 4:26 AM

Yeah, looks good.

Krasimir, any further concerns?

This revision is now accepted and ready to land.Jun 14 2017, 4:26 AM
Typz marked an inline comment as done.Jun 14 2017, 4:31 AM
Typz updated this revision to Diff 102525.Jun 14 2017, 4:31 AM

Make tests more readable

Looks good, except for the tests that actually add or fix namespace end comments should be moved to NamespaceEndCommentsFixerTest.cpp.

krasimir added inline comments.Jun 14 2017, 4:37 AM
unittests/Format/FormatTest.cpp
1394 ↗(On Diff #102525)

Re-indent this line.

Typz updated this revision to Diff 102528.Jun 14 2017, 5:16 AM
Typz marked an inline comment as done.

Move tests that add or fix namespace end comments to NamespaceEndCommentsFixerTest.cpp

This revision was automatically updated to reflect the committed changes.