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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.


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


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:139
+
+bool isEndOfNamespace(const AnnotatedLine *line,
+                      const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
----------------
djasper wrote:
> s/line/Line/
these methods (isEndOfNamespace, ...) are somewhat duplicated in UnwrapperLinesFormatter and NamespaceEndComments (not really, but very similar) : should I factorize these into helper methods of FormatToken?


https://reviews.llvm.org/D32480