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&id=102531#toc
Repository:
rL LLVM
https://reviews.llvm.org/D
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
lib/Fo
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
cfe-commits@lists.llvm.org
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
lib/Format/UnwrappedLineP
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
http
klimek added a comment.
Generally LG from my side.
Comment at: unittests/Format/FormatTest.cpp:1363-1381
+ EXPECT_EQ("namespace {\n"
+ "namespace {\n"
+"}} // namespace
aaa
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
lib/Format/
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 we
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.
https://reviews.llvm.org/D3
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 merg
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.
In https://reviews.llvm.org/D32480#761862, @krasimir wrote:
> In any case, adding a namespace end comment to a line closing multiple
> namespaces is super confusing for me: what does the comment refer to: the
> inner one, the outer one, or both?
> `}} // namespace A::B`
Typz added a comment.
In https://reviews.llvm.org/D32480#761859, @krasimir wrote:
> This change should also adapt NamespaceEndCommentFixer to respect the new
> option and not introduce/remove/change the comments unexpectedly.
This should be handled already (pending further changes to the patch
krasimir added a comment.
In any case, adding a namespace end comment to a line closing multiple
namespaces is super confusing for me: what does the comment refer to: the inner
one, the outer one, or both?
`}} // namespace A::B`
https://reviews.llvm.org/D32480
__
Typz added a comment.
Just to be clear: as it is, the patch does not implement what you describe,
both on the 'indent' side (as expected, we are discussing it), but also on the
"merging" of braces side. Consecutive namespace opening are merged on one side;
and consecutive namespace closing are
krasimir added a comment.
This change should also adapt NamespaceEndCommentFixer to respect the new
option and not introduce/remove/change the comments unexpectedly.
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llv
djasper added a comment.
That's what I meant by "The name NamespaceIndentation might then be a bit
confusing, but not sure whether it's worth changing it.".
I am honestly not sure. Let's get a third opinion.
If we add this additional option, I think we need to fix the behavior and make
what pe
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 indent
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
cfe-comm
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 Compact
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
lib/Format/NamespaceEndCo
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 s
30 matches
Mail list logo
