[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:
> > > 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.
> 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?
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?


https://reviews.llvm.org/D32480