Open tiagomacarios opened 2 months ago
Hi!
This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:
test/
create fine-grained testing targets, so you can e.g. use make check-clang-ast
to only run Clang's AST tests.git clang-format HEAD~1
to format your changes.If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.
@llvm/issue-subscribers-good-first-issue
Author: Tiago (tiagomacarios)
I would recommend supporting
namespace A { namespace B { class C; }}
as well as
namespace A { class C; }}
I am able to pick this up, however there is an edge case I'm not sure how to solve:
// What happens if we format with '{BasedOnStyle: llvm, ColumnLimit: 30}'?
namespace A { namespace B { class C; }}
// Option 1
namespace A {
namespace B {
class C;
}
}
//Option 2 - I assume this one
namespace A {
namespace B { class C; }
}
Our main desire for AllowShortNamespacesOnASingleLine
is to reduce the number of lines these declarations take up. I agree that Option 2 most closely aligns with that goal.
We're also conscious that clang-tidy's modernize-concat-nested-namespaces could be a useful tool to prevent hitting column limits in this scenario.
Yah, also just to point it out, since it might not be super obvious, but the nested namespaces also really doesn't play nice with the FixNamespaceComments
option. To demonstrate:
namespace A { namespace B { namespace C { class D; } } }
// Becomes this, instead of a no-op. Even worse with ShortNamespaceLines: 0
namespace A {
namespace B { namespace C { class D; } } // namespace B
} // namespace A
My current plan is that this will be the output, but really it seems very awkward to reconcile AllowShortNamespacesOnASingleLine
with FixNamespaceComments
and nested namespaces.
clang-format: Add "AllowShortNamespacesOnASingleLine" option Proposed adding a AllowShortNamespacesOnASingleLine option, but the patch did not make it in. Would it be possible for us to consider applying that patch or looking at other ways to accomplish the same functionality?