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