djasper added inline comments.
================
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
----------------
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?
https://reviews.llvm.org/D32480
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
