[PATCH] D113319: [clang-format] Improve require and concept handling
Marek Kurdej via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 4 14:14:00 PST 2022
curdeius added a comment.
Woow, that's a mountain of work!
All my comments are non-blocking.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:1992
+**BreakBeforeConceptDeclarations** (``BreakBeforeConceptDeclarationsStyle``) :versionbadge:`clang-format 13`
+ The style wether before ``concept`` declarations should be broken.
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:1997
+ * ``BBCDS_Never`` (in configuration: ``Never``)
+ Never break, put them in the template declaration line.
+
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2004-2005
+ * ``BBCDS_Allowed`` (in configuration: ``Allowed``)
+ It can be broken, but doesn't have to. The actual behavior depends on
+ the content and line breaking rules and penalities.
+
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2008
+ * ``BBCDS_Always`` (in configuration: ``Always``)
+ Always break, put them in the line after the template declaration.
+
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2708-2709
+**IndentRequiresClause** (``Boolean``) :versionbadge:`clang-format 13`
+ Indent the requires clause in a template. This only applies
+ ``RequiresClausePosition`` is ``OwnLine``, or ``ToFollowing``.
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3498
+ * ``RCPS_OwnLine`` (in configuration: ``OwnLine``)
+ The clause always gets its own line.
+
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3515
+
+ * ``RCPS_ToPreceding`` (in configuration: ``ToPreceding``)
+ The clause tries to stick to the template declaration in case of class
----------------
Maybe `WithPreceding` makes a little more sense without context?
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3516-3518
+ The clause tries to stick to the template declaration in case of class
+ templates or between template and function declarations. In case of
+ after the function declaration it tries to stick to this.
----------------
Just a suggestion, I'm not a writer. I'd just like to see something clear and comprehensible.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3532
+
+ * ``RCPS_ToFollowing`` (in configuration: ``ToFollowing``)
+ The clause tries to stick to the class respectively function
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3533-3534
+ * ``RCPS_ToFollowing`` (in configuration: ``ToFollowing``)
+ The clause tries to stick to the class respectively function
+ declaration.
+
----------------
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3549-3550
+ * ``RCPS_SingleLine`` (in configuration: ``SingleLine``)
+ The clause tries to get everything in the same line, if it doesn't fit
+ normal line breaking rules decide to which part it sticks.
+
----------------
I'm really not fond of writing that "clauses try to do something" :).
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3554-3558
+ template<typename T> requires C<T> struct Foo {...
+
+ template<typename T> requires C<T> void bar(T t) {...
+
+ template<typename T> void bar(T t) requires C<T> {...
----------------
It would be cool to have an example with long template, long parameter list and a long requires clause too.
================
Comment at: clang/docs/ReleaseNotes.rst:157
+- **Important Change** Renamed ``IndentRequires`` to ``IndentRequiresClause``
+ and changed the default for all styles from ``false`` to ``true``.
----------------
================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:484
// different LineFormatter would be used otherwise.
- if (Previous.ClosesTemplateDeclaration)
+ // *: Except when antoher option does interfere with that, like concepts.
+ if (Previous.ClosesTemplateDeclaration) {
----------------
================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:1499-1500
+ if (State.NextToken->ClosesRequiresClause && Style.IndentRequiresClause) {
+ // Remove the indentation of the requires clauses (which is not in Indent,
+ // but in LastSpace).
+ State.Stack.back().LastSpace -= Style.IndentWidth;
----------------
And why it is not in `Indent`?
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3900-3908
+ if (Right.is(TT_RequiresClause)) {
+ switch (Style.RequiresClausePosition) {
+ case FormatStyle::RCPS_OwnLine:
+ case FormatStyle::RCPS_ToFollowing:
+ return true;
+ default:
+ break;
----------------
The body of seems to be the exact same code as below in lines 3920-3926.
Maybe you can refactor to something like this?
Name of the lambda in my suggestion is maybe incorrect though.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3919-3927
+ if (Left.ClosesRequiresClause) {
+ switch (Style.RequiresClausePosition) {
+ case FormatStyle::RCPS_OwnLine:
+ case FormatStyle::RCPS_ToPreceding:
+ return true;
+ default:
+ break;
----------------
And then here as well.
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:439
}
-
-// Returns true if a simple block, or false otherwise. (A simple block has a
-// single statement.)
-bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace, IfStmtKind *IfKind) {
+/// \brief Parses a level, that is ???.
+/// \param HasOpeningBrace If that level is started by an opening brace.
----------------
Haha, I guess it can be anything! :)
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:731
+ bool CanContainBracedList,
+ TokenType NextLBracesType) {
assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
----------------
Why plural?
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:802-804
+ if (FormatTok->is(tok::kw_noexcept))
+ // A noexcept in a requires expression.
+ nextToken();
----------------
As you have a comment, put braces around the body of the if.
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2154
-void UnwrappedLineParser::parseParens() {
+/// \brief Parses a parens pair.
+/// \param AmpAmpTokenType If different than TT_Unknown sets this type for all
----------------
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2767-2768
+///
+/// Returns if it either has finished parsing the concept, or it detects, that
+/// the concept definition is incorrect.
void UnwrappedLineParser::parseConcept() {
----------------
Doesn't it mean that it always returns? :) Not sure what the value of this comment is.
Maybe it should return a `bool` to indicate success/failure?
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2797
+ // of it.
+ bool NotInRequiresExpression =
+ FormatTok->Previous &&
----------------
Not a big fan of a double negation, but it rests acceptable here.
================
Comment at: clang/unittests/Format/FormatTest.cpp:3819-3822
+ verifyFormat("template <int I>\n"
+ "constexpr void foo\n"
+ " requires(I == 42)\n"
+ "{}\n"
----------------
Do you test what was here previously with `RequiresClausePosition: SingleLine` (or whichever relevant option)?
================
Comment at: clang/unittests/Format/FormatTest.cpp:19887
+ BreakBeforeConceptDeclarations, FormatStyle::BBCDS_Allowed);
+ CHECK_PARSE("BreakBeforeConceptDeclarations: true",
+ BreakBeforeConceptDeclarations, FormatStyle::BBCDS_Always);
----------------
Please add a comment that true and false are for backward compatibility. It helps when grepping.
================
Comment at: clang/unittests/Format/FormatTest.cpp:23249-23252
+ verifyFormat("template <typename T>\n"
+ "concept C = [] -> bool { return true; }() && requires(T t) { "
+ "t.bar(); } &&\n"
+ " sizeof(T) <= 8;");
----------------
How about adding a test case with the exact same concept body but surrounded with parens (e.g. using `decltype(...)`)?
The one below looks *almost* the same, but uses `std::true_type` and `::value` and so it's harder to transpose to what this test should look like.
================
Comment at: clang/unittests/Format/FormatTest.cpp:23417
+ " requires Bar<T> && Foo<T>;\n"
+ " requires((trait<T> && Baz) || (T2<T> && Foo<T>));\n"
+ " };",
----------------
Hmm, no space after `requires` here?
It looks strange.
My preference would be not to put a space before a parameter list (as in `requires(T t)`), but put a space before of a constraint expression.
It seems to be the style used in https://en.cppreference.com/w/cpp/language/constraints.
Unless there are options that modify this behaviour?
================
Comment at: clang/unittests/Format/FormatTest.cpp:23417
+ " requires Bar<T> && Foo<T>;\n"
+ " requires((trait<T> && Baz) || (T2<T> && Foo<T>));\n"
+ " };",
----------------
curdeius wrote:
> Hmm, no space after `requires` here?
> It looks strange.
> My preference would be not to put a space before a parameter list (as in `requires(T t)`), but put a space before of a constraint expression.
> It seems to be the style used in https://en.cppreference.com/w/cpp/language/constraints.
> Unless there are options that modify this behaviour?
Oh, I've just seen a comment below on basically the same topic.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113319/new/
https://reviews.llvm.org/D113319
More information about the cfe-commits
mailing list