#6031: Lead-in comments of deleted blocks are now removed along with it

libs/parser/DefBlockSyntaxParser.h

@@ -2,6 +2,7 @@
 
 #include <memory>
 #include <vector>
+#include <algorithm>
 
 #include "string/tokeniser.h"
 #include "string/join.h"
@@ -118,6 +119,12 @@ class DefWhitespaceSyntax :
         assert(token.type == DefSyntaxToken::Type::Whitespace);
     }
 
+    // Returns the number of line breaks this whitespace contains
+    std::size_t getNumberOfLineBreaks()
+    {
+        return std::count_if(_token.value.begin(), _token.value.end(), [](char c) { return c == '\n'; });
+    }
+
     std::string getString() const override
     {
         return _token.value;

radiantcore/decl/DeclarationManager.cpp

@@ -325,6 +325,79 @@ void DeclarationManager::removeDeclaration(Type type, const std::string& name)
     });
 }
 
+namespace
+{
+
+void removeDeclarationFromSyntaxTree(const parser::DefSyntaxTree::Ptr& syntaxTree, const std::string& declName)
+{
+    // Remove the declaration from the tree
+    std::vector<parser::DefSyntaxNode::Ptr> nodesToRemove;
+
+    const auto& childNodes = syntaxTree->getRoot()->getChildren();
+    for (int i = 0; i < childNodes.size(); ++i)
+    {
+        const auto& node = childNodes.at(i);
+
+        if (node->getType() != parser::DefSyntaxNode::Type::DeclBlock) continue;
+
+        auto blockNode = std::static_pointer_cast<parser::DefBlockSyntax>(node);
+
+        if (blockNode->getName() && blockNode->getName()->getToken().value == declName)
+        {
+            nodesToRemove.push_back(blockNode);
+
+            parser::DefSyntaxNode::Ptr encounteredWhitespace;
+
+            // Try to locate comment nodes preceding this block
+            for (int p = i - 1; p >= 0; --p)
+            {
+                const auto& predecessor = childNodes.at(p);
+                if (predecessor->getType() == parser::DefSyntaxNode::Type::Whitespace)
+                {
+                    // Stop at the first whitespace token that contains more than one line break
+                    auto whitespace = std::static_pointer_cast<parser::DefWhitespaceSyntax>(predecessor);
+
+                    // stop searching at the first empty line
+                    if (whitespace->getNumberOfLineBreaks() > 1) break;
+
+                    // This is just a single line break (or none), remember to remove it when we encounter a comment
+                    encounteredWhitespace = predecessor;
+                }
+                else if (predecessor->getType() == parser::DefSyntaxNode::Type::Comment)
+                {
+                    nodesToRemove.push_back(predecessor);
+
+                    // Remove any whitespace that stood between this comment and the block
+                    if (encounteredWhitespace)
+                    {
+                        nodesToRemove.push_back(encounteredWhitespace);
+                        encounteredWhitespace.reset();
+                    }
+
+                    continue;
+                }
+                else // stop at all other node types
+                {
+                    break;
+                }
+            }
+        }
+    }
+
+    if (nodesToRemove.empty())
+    {
+        rWarning() << "Could not locate the decl block " << declName << std::endl;
+        return;
+    }
+
+    for (const auto& node : nodesToRemove)
+    {
+        syntaxTree->getRoot()->removeChildNode(node);
+    }
+}
+
+}
+
 void DeclarationManager::removeDeclarationFromFile(const IDeclaration::Ptr& decl)
 {
     const auto& syntax = decl->getBlockSyntax();
@@ -361,33 +434,9 @@ void DeclarationManager::removeDeclarationFromFile(const IDeclaration::Ptr& decl
     auto syntaxTree = parser.parse();
     inheritStream.close();
 
-    // Remove the declaration from the tree
-    std::vector<parser::DefSyntaxNode::Ptr> nodesToRemove;
-
-    for (const auto& node : syntaxTree->getRoot()->getChildren())
-    {
-        if (node->getType() != parser::DefSyntaxNode::Type::DeclBlock) continue;
-
-        auto blockNode = std::static_pointer_cast<parser::DefBlockSyntax>(node);
-
-        if (blockNode->getName() && blockNode->getName()->getToken().value == decl->getDeclName())
-        {
-            nodesToRemove.push_back(blockNode);
-        }
-    }
-
-    if (nodesToRemove.empty())
-    {
-        rWarning() << "Could not locate the decl block " << decl->getDeclName() << " in the file " << fullPath << std::endl;
-        return;
-    }
-
-    for (const auto& node : nodesToRemove)
-    {
-        syntaxTree->getRoot()->removeChildNode(node);
-    }
+    removeDeclarationFromSyntaxTree(syntaxTree, decl->getDeclName());
 
-    // Export the syntax tree with the missing decl block
+    // Export the modified syntax tree
     stream << syntaxTree->getString();
 
     tempStream.closeAndReplaceTargetFile();

test/DeclManager.cpp

@@ -903,6 +903,46 @@ TEST_F(DeclManagerTest, RemoveDeclarationRemovesLeadingMultilineComment)
     EXPECT_TRUE(algorithm::fileContainsText(fullPath, "// Some comments after decl/removal/2"));
 }
 
+// Multiple comments preceding the decl with minor whitespace in between
+TEST_F(DeclManagerTest, RemoveDeclarationRemovesLeadingMixedComment)
+{
+    GlobalDeclarationManager().registerDeclType("testdecl", std::make_shared<TestDeclarationCreator>());
+    GlobalDeclarationManager().registerDeclFolder(decl::Type::TestDecl, TEST_DECL_FOLDER, ".decl");
+
+    // Create a backup copy of the decl file we're going to manipulate
+    auto fullPath = _context.getTestProjectPath() + "testdecls/removal_tests.decl";
+    BackupCopy backup(fullPath);
+
+    // decl/removal/3a has leading multiline comment, and a trailing single line comment
+    EXPECT_TRUE(algorithm::fileContainsText(fullPath, R"(// Multiple comment lines before decl/removal/a3
+/* mixed style comments too */
+	 // multiple comment with some whitespace at the start of each line
+ // multiple comment with some whitespace at the start of each line)"));
+
+    GlobalDeclarationManager().removeDeclaration(decl::Type::TestDecl, "decl/removal/3a");
+
+    // Leading comment should be gone, trailing comment should stay
+    EXPECT_FALSE(algorithm::fileContainsText(fullPath, R"(decl/removal/3a
+{
+    diffusemap textures/removal/a3
+})"));
+    EXPECT_FALSE(algorithm::fileContainsText(fullPath, R"(// Multiple comment lines before decl/removal/a3
+/* mixed style comments too */
+	 // multiple comment with some whitespace at the start of each line
+ // multiple comment with some whitespace at the start of each line)"));
+
+    // Safety check that nothing else got removed (including whitespace)
+    EXPECT_TRUE(algorithm::fileContainsText(fullPath, R"(
+
+// Comment with a line before the start of the decl decl/removal/5)"));
+    EXPECT_TRUE(algorithm::fileContainsText(fullPath, R"(TestDecl decl/removal/3
+{
+    diffusemap textures/removal/3
+}
+
+)"));
+}
+
 // There's an evil misleading commented out declaration decl/removal/9 in the file
 // right before the actual, uncommented one
 TEST_F(DeclManagerTest, RemoveDeclarationIgnoresCommentedContent)

test/resources/tdm/testdecls/removal_tests.decl

@@ -29,6 +29,15 @@ TestDecl decl/removal/3
     diffusemap textures/removal/3
 }
 
+// Multiple comment lines before decl/removal/a3
+/* mixed style comments too */
+	 // multiple comment with some whitespace at the start of each line
+ // multiple comment with some whitespace at the start of each line
+testDecl decl/removal/3a
+{
+    diffusemap textures/removal/a3
+}
+
 // Comment with a line before the start of the decl decl/removal/5
 
 testdecl decl/removal/5 { // a comment in the same line as the opening brace