CppEditor: Add curly braces for more control statements

Fixes: QTCREATORBUG-24542
Change-Id: I3e0893e1c736730d94e2c9ab2baa0aa580393fe4
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Christian Stenger <christian.stenger@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

src/plugins/cppeditor/cppquickfix_test.cpp

@@ -8229,29 +8229,105 @@ void QuickfixTest::testExtractLiteralAsParameterNotTriggeringForInvalidCode()
     QuickFixOperationTest(testDocuments, &factory);
 }
 
-void QuickfixTest::testAddCurlyBraces()
+void QuickfixTest::testAddCurlyBraces_data()
 {
-    QList<TestDocumentPtr> testDocuments;
+    QTest::addColumn<QByteArray>("original");
-    const QByteArray original = R"delim(
+    QTest::addColumn<QByteArray>("expected");
+
+    QByteArray original = R"delim(
 void MyObject::f()
 {
     @if (true)
         emit mySig();
-}
+})delim";
-)delim";
+    QByteArray expected = R"delim(
-    const QByteArray expected = R"delim(
 void MyObject::f()
 {
     if (true) {
         emit mySig();
     }
+})delim";
+    QTest::newRow("if") << original << expected;
+
+    original = R"delim(
+void MyObject::f()
+{
+    @while (true)
+        emit mySig();
+})delim";
+    expected = R"delim(
+void MyObject::f()
+{
+    while (true) {
+        emit mySig();
+    }
+})delim";
+    QTest::newRow("while") << original << expected;
+
+    original = R"delim(
+void MyObject::f()
+{
+    @for (int i = 0; i < 10; ++i)
+        emit mySig();
+})delim";
+    expected = R"delim(
+void MyObject::f()
+{
+    for (int i = 0; i < 10; ++i) {
+        emit mySig();
+    }
+})delim";
+    QTest::newRow("for") << original << expected;
+
+    original = R"delim(
+void MyObject::f()
+{
+    @for (int i : list)
+        emit mySig();
+})delim";
+    expected = R"delim(
+void MyObject::f()
+{
+    for (int i : list) {
+        emit mySig();
+    }
+})delim";
+    QTest::newRow("range-based for") << original << expected;
+
+    original = R"delim(
+void MyObject::f()
+{
+    @do
+        emit mySig();
+    while (true);
+})delim";
+    expected = R"delim(
+void MyObject::f()
+{
+    do {
+        emit mySig();
+    } while (true);
+})delim";
+    QTest::newRow("do") << original << expected;
+
+    original = R"delim(
+void MyObject::f()
+{
+    @do {
+        emit mySig();
+    } while (true);
+})delim";
+    expected.clear();
+    QTest::newRow("already has braces") << original << expected;
 }
-)delim";
 
-    testDocuments << CppTestDocument::create("file.cpp", original, expected);
+void QuickfixTest::testAddCurlyBraces()
-    AddBracesToIf factory;
+{
-    QuickFixOperationTest(testDocuments, &factory);
+    QFETCH(QByteArray, original);
+    QFETCH(QByteArray, expected);
 
+    AddBracesToControlStatement factory;
+    QuickFixOperationTest({CppTestDocument::create("file.cpp", original, expected)}, &factory);
 }
 
 void QuickfixTest::testConvertQt4ConnectConnectOutOfClass()

src/plugins/cppeditor/cppquickfix_test.h

@@ -209,6 +209,7 @@ private slots:
     void testExtractLiteralAsParameterMemberFunctionSeparateFiles();
     void testExtractLiteralAsParameterNotTriggeringForInvalidCode();
 
+    void testAddCurlyBraces_data();
     void testAddCurlyBraces();
 
     void testRemoveUsingNamespace_data();

src/plugins/cppeditor/cppquickfixes.cpp

@@ -764,13 +764,49 @@ void SplitSimpleDeclaration::doMatch(const CppQuickFixInterface &interface,
 }
 
 namespace {
+template<typename Statement> Statement *asControlStatement(AST *node)
+{
+    if constexpr (std::is_same_v<Statement, IfStatementAST>)
+        return node->asIfStatement();
+    if constexpr (std::is_same_v<Statement, WhileStatementAST>)
+        return node->asWhileStatement();
+    if constexpr (std::is_same_v<Statement, ForStatementAST>)
+            return node->asForStatement();
+    if constexpr (std::is_same_v<Statement, RangeBasedForStatementAST>)
+        return node->asRangeBasedForStatement();
+    if constexpr (std::is_same_v<Statement, DoStatementAST>)
+        return node->asDoStatement();
+    return nullptr;
+}
 
-class AddBracesToIfOp: public CppQuickFixOperation
+template<typename Statement>
+int triggerToken(const Statement *statement)
+{
+    if constexpr (std::is_same_v<Statement, IfStatementAST>)
+        return statement->if_token;
+    if constexpr (std::is_same_v<Statement, WhileStatementAST>)
+        return statement->while_token;
+    if constexpr (std::is_same_v<Statement, DoStatementAST>)
+        return statement->do_token;
+    if constexpr (std::is_same_v<Statement, ForStatementAST>
+                  || std::is_same_v<Statement, RangeBasedForStatementAST>) {
+        return statement->for_token;
+    }
+}
+
+template<typename Statement>
+int tokenToInsertOpeningBraceAfter(const Statement *statement)
+{
+    if constexpr (std::is_same_v<Statement, DoStatementAST>)
+        return statement->do_token;
+    return statement->rparen_token;
+}
+
+template<typename Statement> class AddBracesToControlStatementOp : public CppQuickFixOperation
 {
 public:
-    AddBracesToIfOp(const CppQuickFixInterface &interface, int priority,
+    AddBracesToControlStatementOp(const CppQuickFixInterface &interface, const Statement *statement)
-                    const IfStatementAST *statement)
+        : CppQuickFixOperation(interface, 0)
-        : CppQuickFixOperation(interface, priority)
         , _statement(statement)
     {
         setDescription(Tr::tr("Add Curly Braces"));
@@ -783,51 +819,58 @@ public:
 
         ChangeSet changes;
 
-        const int start = currentFile->endOf(_statement->rparen_token);
+        const int start = currentFile->endOf(tokenToInsertOpeningBraceAfter(_statement));
         changes.insert(start, QLatin1String(" {"));
 
+        if constexpr (std::is_same_v<Statement, DoStatementAST>) {
+            const int end = currentFile->startOf(_statement->while_token);
+            changes.insert(end, QLatin1String("} "));
+        } else {
             const int end = currentFile->endOf(_statement->statement->lastToken() - 1);
             changes.insert(end, QLatin1String("\n}"));
+        }
 
+        // TODO: For if statements, also bracify all else cases.
+        //       Also check all else cases in the factory.
         currentFile->setChangeSet(changes);
         currentFile->apply();
     }
 
 private:
-    const IfStatementAST * const _statement;
+    const Statement * const _statement;
 };
 
 } // anonymous namespace
 
-void AddBracesToIf::doMatch(const CppQuickFixInterface &interface, QuickFixOperations &result)
+template<typename Statement>
+bool checkControlStatementsHelper(const CppQuickFixInterface &interface, QuickFixOperations &result)
 {
-    const QList<AST *> &path = interface.path();
+    Statement * const statement = asControlStatement<Statement>(interface.path().last());
-    if (path.isEmpty())
+    if (!statement)
-        return;
+        return false;
 
-    // show when we're on the 'if' of an if statement
+    if (interface.isCursorOn(triggerToken(statement)) && statement->statement
-    int index = path.size() - 1;
+        && !statement->statement->asCompoundStatement()) {
-    IfStatementAST *ifStatement = path.at(index)->asIfStatement();
+        result << new AddBracesToControlStatementOp(interface, statement);
-    if (ifStatement && interface.isCursorOn(ifStatement->if_token) && ifStatement->statement
-        && !ifStatement->statement->asCompoundStatement()) {
-        result << new AddBracesToIfOp(interface, index, ifStatement);
-        return;
     }
+    return true;
+}
 
-    // or if we're on the statement contained in the if
+template<typename ...Statements>
-    // ### This may not be such a good idea, consider nested ifs...
+void checkControlStatements(const CppQuickFixInterface &interface, QuickFixOperations &result)
-    for (; index != -1; --index) {
+{
-        IfStatementAST *ifStatement = path.at(index)->asIfStatement();
+    (... || checkControlStatementsHelper<Statements>(interface, result));
-        if (ifStatement && ifStatement->statement
+}
-            && interface.isCursorOn(ifStatement->statement)
-            && !ifStatement->statement->asCompoundStatement()) {
-            result << new AddBracesToIfOp(interface, index, ifStatement);
-            return;
-        }
-    }
 
-    // ### This could very well be extended to the else branch
+void AddBracesToControlStatement::doMatch(const CppQuickFixInterface &interface, QuickFixOperations &result)
-    // and other nodes entirely.
+{
+    if (interface.path().isEmpty())
+        return;
+    checkControlStatements<IfStatementAST,
+                           WhileStatementAST,
+                           ForStatementAST,
+                           RangeBasedForStatementAST,
+                           DoStatementAST>(interface, result);
 }
 
 namespace {
@@ -9969,7 +10012,7 @@ void createCppQuickFixes()
     new SplitIfStatement;
     new SplitSimpleDeclaration;
 
-    new AddBracesToIf;
+    new AddBracesToControlStatement;
     new RearrangeParamDeclarationList;
     new ReformatPointerDeclaration;
 

src/plugins/cppeditor/cppquickfixes.h

@@ -287,7 +287,7 @@ public:
 };
 
 /*!
-  Add curly braces to a if statement that doesn't already contain a
+  Add curly braces to a control statement that doesn't already contain a
   compound statement. I.e.
 
   if (a)
@@ -297,9 +297,9 @@ public:
       b;
   }
 
-  Activates on: the if
+  Activates on: the keyword
 */
-class AddBracesToIf: public CppQuickFixFactory
+class AddBracesToControlStatement : public CppQuickFixFactory
 {
 public:
     void doMatch(const CppQuickFixInterface &interface, QuickFixOperations &result) override;