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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
