CppEditor: Consider unexpanded token ranges when refactoring

When moving or deleting pieces of code, we are normally interested in
the actual content of the file in the respective locations, not in what
any macros present there will expand to.
This supersedes commit 76ae5780c4.

Fixes: QTCREATORBUG-10279
Change-Id: I0fb547b23244cd5875e80c019a3595f3f9c33d52
Reviewed-by: Christian Stenger <christian.stenger@qt.io>

src/libs/3rdparty/cplusplus/TranslationUnit.cpp

@@ -27,7 +27,7 @@
 #include "Literals.h"
 #include "DiagnosticClient.h"
 
-#include "cppassert.h"
+#include <utils/qtcassert.h>
 #include <utils/textutils.h>
 
 #include <stack>
@@ -88,7 +88,7 @@ int TranslationUnit::sourceLength() const
 
 void TranslationUnit::setSource(const char *source, int size)
 {
-    CPP_CHECK(source);
+    QTC_ASSERT(source, return);
     _firstSourceChar = source;
     _lastSourceChar = source + size;
 }
@@ -191,6 +191,8 @@ void TranslationUnit::tokenize()
     int lineColumnIdx = 0;
 
     Token tk;
+    int macroOffset = -1;
+    int macroLength = -1;
     do {
         lex(&tk);
 
@@ -209,17 +211,12 @@ recognize:
                         lex(&tk);
 
                         // Gather where the expansion happens and its length.
-                        //int macroOffset = static_cast<int>(strtoul(tk.spell(), 0, 0));
+                        macroOffset = static_cast<int>(strtoul(tk.spell(), 0, 0));
                         lex(&tk);
                         lex(&tk); // Skip the separating comma
-                        //int macroLength = static_cast<int>(strtoul(tk.spell(), 0, 0));
+                        macroLength = static_cast<int>(strtoul(tk.spell(), 0, 0));
                         lex(&tk);
 
-                        // NOTE: We are currently not using the macro offset and length. They
-                        // are kept here for now because of future use.
-                        //Q_UNUSED(macroOffset)
-                        //Q_UNUSED(macroLength)
-
                         // Now we need to gather the real line and columns from the upcoming
                         // tokens. But notice this is only relevant for tokens which are expanded
                         // but not generated.
@@ -307,6 +304,11 @@ recognize:
         tk.f.generated = currentGenerated;
 
         _tokens->push_back(tk);
+
+        if (currentExpanded) {
+            QTC_ASSERT(macroOffset != -1 && macroLength != -1, continue);
+            _expansionPositions[_tokens->size() - 1] = std::make_pair(macroOffset, macroLength);
+        }
     } while (tk.kind());
 
     for (; ! braces.empty(); braces.pop()) {
@@ -462,6 +464,14 @@ int TranslationUnit::getTokenEndPositionInDocument(const Token &token,
     return Utils::Text::positionInText(doc, line, column);
 }
 
+std::pair<int, int> TranslationUnit::getExpansionPosition(int tokenIndex) const
+{
+    QTC_ASSERT(tokenIndex < int(_tokens->size()) && tokenAt(tokenIndex).generated(), return {});
+    const auto it = _expansionPositions.find(tokenIndex);
+    QTC_ASSERT(it != _expansionPositions.end(), return {});
+    return it->second;
+}
+
 void TranslationUnit::getPosition(int utf16charOffset,
                                   int *line,
                                   int *column,

src/libs/3rdparty/cplusplus/TranslationUnit.h

@@ -133,6 +133,7 @@ public:
                              const StringLiteral **fileName = nullptr) const;
     int getTokenPositionInDocument(const Token token, const QTextDocument *doc) const;
     int getTokenEndPositionInDocument(const Token &token, const QTextDocument *doc) const;
+    std::pair<int, int> getExpansionPosition(int tokenIndex) const;
 
     void pushLineOffset(int offset);
     void pushPreprocessorLine(int utf16charOffset,
@@ -183,6 +184,11 @@ private:
     std::vector<Token> *_comments;
     std::vector<int> _lineOffsets;
     std::vector<PPLine> _ppLines;
+
+    // Offset and length. Note that in contrast to token offsets, this is a raw file offset
+    // with no preprocessor prefix.
+    std::unordered_map<int, std::pair<int, int>> _expansionPositions;
+
     typedef std::unordered_map<unsigned, std::pair<int, int> > TokenLineColumn;
     TokenLineColumn _expandedLineColumn;
     MemoryPool *_pool;

src/plugins/cppeditor/cpprefactoringchanges.cpp

@@ -221,6 +221,9 @@ ChangeSet::Range CppRefactoringFile::range(const AST *ast) const
 
 int CppRefactoringFile::startOf(unsigned index) const
 {
+    if (const auto loc = expansionLoc(index))
+        return loc->first;
+
     int line, column;
     cppDocument()->translationUnit()->getPosition(tokenAt(index).utf16charsBegin(), &line, &column);
     return document()->findBlockByNumber(line - 1).position() + column - 1;
@@ -229,15 +232,14 @@ int CppRefactoringFile::startOf(unsigned index) const
 int CppRefactoringFile::startOf(const AST *ast) const
 {
     QTC_ASSERT(ast, return 0);
-    int firstToken = ast->firstToken();
+    return startOf(ast->firstToken());
-    const int lastToken = ast->lastToken();
-    while (tokenAt(firstToken).generated() && firstToken < lastToken)
-        ++firstToken;
-    return startOf(firstToken);
 }
 
 int CppRefactoringFile::endOf(unsigned index) const
 {
+    if (const auto loc = expansionLoc(index))
+        return loc->first + loc->second;
+
     int line, column;
     cppDocument()->translationUnit()->getPosition(tokenAt(index).utf16charsEnd(), &line, &column);
     return document()->findBlockByNumber(line - 1).position() + column - 1;
@@ -248,14 +250,17 @@ int CppRefactoringFile::endOf(const AST *ast) const
     QTC_ASSERT(ast, return 0);
     int lastToken = ast->lastToken() - 1;
     QTC_ASSERT(lastToken >= 0, return -1);
-    const int firstToken = ast->firstToken();
-    while (tokenAt(lastToken).generated() && lastToken > firstToken)
-        --lastToken;
     return endOf(lastToken);
 }
 
 void CppRefactoringFile::startAndEndOf(unsigned index, int *start, int *end) const
 {
+    if (const auto loc = expansionLoc(index)) {
+        *start = loc->first;
+        *end = loc->first + loc->second;
+        return;
+    }
+
     int line, column;
     Token token(tokenAt(index));
     cppDocument()->translationUnit()->getPosition(token.utf16charsBegin(), &line, &column);
@@ -263,6 +268,13 @@ void CppRefactoringFile::startAndEndOf(unsigned index, int *start, int *end) con
     *end = *start + token.utf16chars();
 }
 
+std::optional<std::pair<int, int> > CppRefactoringFile::expansionLoc(unsigned int index) const
+{
+    if (!tokenAt(index).expanded())
+        return {};
+    return cppDocument()->translationUnit()->getExpansionPosition(index);
+}
+
 QString CppRefactoringFile::textOf(const AST *ast) const
 {
     return textOf(startOf(ast), endOf(ast));

src/plugins/cppeditor/cpprefactoringchanges.h

@@ -11,6 +11,8 @@
 
 #include <texteditor/refactoringchanges.h>
 
+#include <optional>
+
 namespace CppEditor {
 class CppRefactoringChanges;
 class CppRefactoringFile;
@@ -30,7 +32,6 @@ public:
     bool isCursorOn(unsigned tokenIndex) const;
     bool isCursorOn(const CPlusPlus::AST *ast) const;
 
-    Range range(int start, int end) const;
     Range range(unsigned tokenIndex) const;
     Range range(const CPlusPlus::AST *ast) const;
 
@@ -43,6 +44,8 @@ public:
 
     void startAndEndOf(unsigned index, int *start, int *end) const;
 
+    std::optional<std::pair<int, int>> expansionLoc(unsigned index) const;
+
     QList<CPlusPlus::Token> tokensForCursor() const;
     QList<CPlusPlus::Token> tokensForCursor(const QTextCursor &cursor) const;
     QList<CPlusPlus::Token> tokensForLine(int line) const;

src/plugins/cppeditor/quickfixes/logicaloperationquickfixes.cpp

@@ -7,6 +7,11 @@
 #include "../cpprefactoringchanges.h"
 #include "cppquickfix.h"
 
+#ifdef WITH_TESTS
+#include "cppquickfix_test.h"
+#include <QtTest>
+#endif
+
 using namespace CPlusPlus;
 using namespace Utils;
 
@@ -149,7 +154,7 @@ class FlipLogicalOperands : public CppQuickFixFactory
 {
 #ifdef WITH_TESTS
 public:
-    static QObject *createTest() { return new QObject; }
+    static QObject *createTest();
 #endif
 
 private:
@@ -311,6 +316,55 @@ private:
     }
 };
 
+#ifdef WITH_TESTS
+using namespace Tests;
+class FlipLogicalOperandsTest : public QObject
+{
+    Q_OBJECT
+
+private slots:
+    void test_data()
+    {
+        QTest::addColumn<QByteArray>("original");
+        QTest::addColumn<QByteArray>("expected");
+
+        const auto makeDoc = [](const QString &expr) {
+            const QString pattern = "#define VALUE 7\n"
+                                    "int main() {\n"
+                                    "    if (%1)\n"
+                                    "        return 1;\n"
+                                    "}\n";
+            return pattern.arg(expr).toUtf8();
+        };
+
+        QTest::newRow("macro as left expr")
+            << makeDoc("VALUE @&& true")
+            << makeDoc("true && VALUE");
+        QTest::newRow("macro in left expr")
+            << makeDoc("(VALUE + 1) @&& true")
+            << makeDoc("true && (VALUE + 1)");
+        QTest::newRow("macro as right expr")
+            << makeDoc("false @|| VALUE")
+            << makeDoc("VALUE || false");
+        QTest::newRow("macro in right expr")
+            << makeDoc("false @|| (VALUE + 1)")
+            << makeDoc("(VALUE + 1) || false");
+    }
+
+    void test()
+    {
+        QFETCH(QByteArray, original);
+        QFETCH(QByteArray, expected);
+
+        FlipLogicalOperands factory;
+        QuickFixOperationTest(singleDocument(original, expected), &factory);
+    }
+};
+
+QObject *FlipLogicalOperands::createTest() { return new FlipLogicalOperandsTest; }
+
+#endif // WITH_TESTS
+
 } // namespace
 
 void registerLogicalOperationQuickfixes()
@@ -321,3 +375,7 @@ void registerLogicalOperationQuickfixes()
 }
 
 } // namespace CppEditor::Internal
+
+#ifdef WITH_TESTS
+#include <logicaloperationquickfixes.moc>
+#endif