This revision was automatically updated to reflect the committed changes.
Closed by commit rL305384: clang-format: Add CompactNamespaces option (authored
by Typz).
Changed prior to commit:
https://reviews.llvm.org/D32480?vs=102528=102531#toc
Repository:
rL LLVM
Typz updated this revision to Diff 102528.
Typz marked an inline comment as done.
Typz added a comment.
Move tests that add or fix namespace end comments to
NamespaceEndCommentsFixerTest.cpp
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
krasimir added inline comments.
Comment at: unittests/Format/FormatTest.cpp:1394
+ "int j;\n"
+ "}\n"
+ "}",
Re-indent this line.
https://reviews.llvm.org/D32480
krasimir added a comment.
Looks good, except for the tests that actually add or fix namespace end
comments should be moved to NamespaceEndCommentsFixerTest.cpp.
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
Typz updated this revision to Diff 102525.
Typz added a comment.
Make tests more readable
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.
Yeah, looks good.
Krasimir, any further concerns?
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
klimek added a comment.
Generally LG from my side.
Comment at: unittests/Format/FormatTest.cpp:1363-1381
+ EXPECT_EQ("namespace {\n"
+ "namespace {\n"
+"}} // namespace
Typz updated this revision to Diff 102346.
Typz added a comment.
- make "compacted" namespaces always add at most one level of indentation
- compact only namespaces which both start and end on consecutive lines
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
djasper added a comment.
Ok. Works for me.
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
klimek added a comment.
In https://reviews.llvm.org/D32480#773807, @Typz wrote:
> So how do I proceed?
>
> 1. Keep the CompactNamespace option, and make "compacted" namespaces always
> add at most one level of indentation
> 2. Or assume that this can only ever usefully work with the behavior of
Typz added a comment.
ping?
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz added a comment.
So how do I proceed?
1. Keep the CompactNamespace option, and make "compacted" namespaces always add
at most one level of indentation
2. Or assume that this can only ever usefully work with the behavior of NI_None
and add an additional enum value NI_Compact.
And should
Typz added a comment.
That will be slightly more complicated to check, esp. we will need to keep
track of the matching-closing-brace (currently only the matching-opening-brace)
is stored.
But I can try to update the patch in that direction, if that is the consensus.
klimek added a comment.
I'm less concerned about everything suddenly re-indenting when you change code
- if you use any kind of namespace indentation, that's what will happen now and
then (and is why many style guides do not indent in namespaces).
For what it's worth, I'd
1. vote for only
Typz added a comment.
ping?
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz updated this revision to Diff 99763.
Typz added a comment.
Remove dependency on https://reviews.llvm.org/D33314
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
Typz added a comment.
Merging the 2 options is definitely a "safe" option, as it prevents ensures
only the most obvious behavior is accessible.
However, it has significant (IMO) drawbacks:
- "Compact" is a not an namespace indentation type, this will make the option
quite confusing
- If
djasper added a comment.
What I mean is that you should remove the CompactNamespace option and instead
let this be configured by an additional enum value in NamespaceIndentation.
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
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
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
> > >
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?
Typz updated this revision to Diff 99575.
Typz marked 11 inline comments as done.
Typz edited the summary of this revision.
Typz added a comment.
update for review comments
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
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
Typz marked 2 inline comments as done.
Typz added inline comments.
Comment at: include/clang/Format/Format.h:153
+ /// \endcode
+ bool AllowSemicolonAfterNamespace;
+
Typz wrote:
> djasper wrote:
> > While I am not entirely opposed to this feature, I think it
24 matches
Mail list logo
