[PATCH] D32480: clang-format: Add CompactNamespaces option

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: include/clang/Format/Format.h:358
+  /// \endcode
+  bool BinPackNamespaces;
+
----------------
Typz wrote:
> djasper wrote:
> > 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)?
> 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.
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.


================
Comment at: include/clang/Format/Format.h:769
+  /// If it does not fit on a single line, the overflowing namespaces get wrapped:
+  /// /// \code
+  ///   namespace Foo { namespace Bar {
----------------
What happened here?


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:124
+
+const FormatToken * getNamespaceToken(const AnnotatedLine *line,
+                                      const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
----------------
no space after "*"..


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:127
+    if (!line->Affected || line->InPPDirective || !line->startsWith(tok::r_brace))
+      return NULL;
+    size_t StartLineIndex = line->MatchingOpeningBlockLineIndex;
----------------
s/NULL/nullptr/


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:172
     }
+    if (StartLineIndex == SIZE_MAX)
+      StartLineIndex = EndLine->MatchingOpeningBlockLineIndex;
----------------
Do we need any of this if CompactNamespaces if false? Maybe we should surround the entire thing with an

  if (Style.CompactNamespaces) {




================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:130
 
+bool isNamespaceToken(const FormatToken *NamespaceTok) {
+  // Detect "(inline)? namespace" in the beginning of a line.
----------------
You always seem to call this function with Line->First. Maybe just call it on an AnnotatedLine?


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:134
+    NamespaceTok = NamespaceTok->getNextNonComment();
+  if (!NamespaceTok || NamespaceTok->isNot(tok::kw_namespace))
+    return false;
----------------
just

  return NamespaceTok && NamespaceTok->is(tok::kw_namespace);


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:139
+
+bool isEndOfNamespace(const AnnotatedLine *line,
+                      const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
----------------
s/line/Line/


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:221
+      if (isNamespaceToken(TheLine->First)) {
+        int i = 0;
+        while (I + 1 + i != E && isNamespaceToken(I[i + 1]->First) &&
----------------
This looks like you wanted to write a for loop.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:223
+        while (I + 1 + i != E && isNamespaceToken(I[i + 1]->First) &&
+            I[i + 1]->Last->TotalLength < Limit) {
+          Limit -= I[i + 1]->Last->TotalLength;
----------------
Indentation


================
Comment at: unittests/Format/FormatTest.cpp:1310
+
+  FormatStyle LLVMWithCompactNamespaces = getLLVMStyle();
+  LLVMWithCompactNamespaces.CompactNamespaces = true;
----------------
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


https://reviews.llvm.org/D32480