CppEditor: Do not add redundant access specifier

... when inserting a member function declaration for a definition.

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

src/plugins/cppeditor/cppquickfix_test.cpp

@@ -4707,6 +4707,9 @@ void insertToSectionDeclFromDef(const QByteArray &section, int sectionIndex)
 
     QByteArray original;
     QByteArray expected;
+    QByteArray sectionString = section + ":\n";
+    if (sectionIndex == 4)
+        sectionString.clear();
 
     // Header File
     original =
@@ -4716,7 +4719,7 @@ void insertToSectionDeclFromDef(const QByteArray &section, int sectionIndex)
     expected =
         "class Foo\n"
         "{\n"
-        + section + ":\n" +
+        + sectionString +
         "    Foo();\n"
         "@};\n";
     testDocuments << QuickFixTestDocument::create("file.h", original, expected);

src/plugins/cppeditor/cppquickfixes.cpp

@@ -3962,9 +3962,9 @@ protected:
         const auto insertionPoint = m_headerInsertionPoints.find(spec);
         if (insertionPoint != m_headerInsertionPoints.end())
             return *insertionPoint;
-        const InsertionLocation loc = m_locator.methodDeclarationInClass(m_headerFile->fileName(),
-                                                                         m_class,
-                                                                         spec);
+        const InsertionLocation loc = m_locator.methodDeclarationInClass(
+                    m_headerFile->fileName(), m_class, spec,
+                    InsertionPointLocator::ForceAccessSpec::Yes);
         m_headerInsertionPoints.insert(spec, loc);
         return loc;
     }

src/plugins/cpptools/insertionpointlocator.cpp

@@ -118,6 +118,7 @@ private:
 void findMatch(const QList<AccessRange> &ranges,
                InsertionPointLocator::AccessSpec xsSpec,
                InsertionPointLocator::Position positionInAccessSpec,
+               InsertionPointLocator::ForceAccessSpec forceAccessSpec,
                unsigned &beforeToken,
                bool &needsLeadingEmptyLine,
                bool &needsPrefix,
@@ -128,8 +129,10 @@ void findMatch(const QList<AccessRange> &ranges,
     const bool atEnd = positionInAccessSpec == InsertionPointLocator::AccessSpecEnd;
     needsLeadingEmptyLine = false;
 
-    // try an exact match, and ignore the first (default) access spec:
-    for (int i = lastIndex; i > 0; --i) {
+    // Try an exact match. Ignore the default access spec unless there is no explicit one.
+    const int firstIndex = ranges.size() == 1
+            && forceAccessSpec == InsertionPointLocator::ForceAccessSpec::No ? 0 : 1;
+    for (int i = lastIndex; i >= firstIndex; --i) {
         const AccessRange &range = ranges.at(i);
         if (range.xsSpec == xsSpec) {
             beforeToken = atEnd ? range.end : range.beforeStart;
@@ -270,23 +273,25 @@ InsertionPointLocator::InsertionPointLocator(const CppRefactoringChanges &refact
 InsertionLocation InsertionPointLocator::methodDeclarationInClass(
     const QString &fileName,
     const Class *clazz,
-    AccessSpec xsSpec) const
+    AccessSpec xsSpec,
+    ForceAccessSpec forceAccessSpec) const
 {
     const Document::Ptr doc = m_refactoringChanges.file(fileName)->cppDocument();
     if (doc) {
         FindInClass find(doc->translationUnit(), clazz);
         ClassSpecifierAST *classAST = find();
-        return methodDeclarationInClass(doc->translationUnit(), classAST, xsSpec);
+        return methodDeclarationInClass(doc->translationUnit(), classAST, xsSpec, AccessSpecEnd,
+                                        forceAccessSpec);
     } else {
         return InsertionLocation();
     }
 }
 
-InsertionLocation InsertionPointLocator::methodDeclarationInClass(
-    const TranslationUnit *tu,
+InsertionLocation InsertionPointLocator::methodDeclarationInClass(const TranslationUnit *tu,
     const ClassSpecifierAST *clazz,
     InsertionPointLocator::AccessSpec xsSpec,
-    Position pos) const
+    Position pos,
+    ForceAccessSpec forceAccessSpec) const
 {
     if (!clazz)
         return {};
@@ -302,7 +307,8 @@ InsertionLocation InsertionPointLocator::methodDeclarationInClass(
     bool needsLeadingEmptyLine = false;
     bool needsPrefix = false;
     bool needsSuffix = false;
-    findMatch(ranges, xsSpec, pos, beforeToken, needsLeadingEmptyLine, needsPrefix, needsSuffix);
+    findMatch(ranges, xsSpec, pos, forceAccessSpec, beforeToken, needsLeadingEmptyLine,
+              needsPrefix, needsSuffix);
 
     int line = 0, column = 0;
     if (pos == InsertionPointLocator::AccessSpecEnd)

src/plugins/cpptools/insertionpointlocator.h

@@ -91,17 +91,25 @@ public:
         AccessSpecEnd,
     };
 
+    enum class ForceAccessSpec { Yes, No };
+
 public:
     explicit InsertionPointLocator(const CppRefactoringChanges &refactoringChanges);
 
-    InsertionLocation methodDeclarationInClass(const QString &fileName,
+    InsertionLocation methodDeclarationInClass(
+            const QString &fileName,
             const CPlusPlus::Class *clazz,
-                                               AccessSpec xsSpec) const;
+            AccessSpec xsSpec,
+            ForceAccessSpec forceAccessSpec = ForceAccessSpec::No
+            ) const;
 
-    InsertionLocation methodDeclarationInClass(const CPlusPlus::TranslationUnit *tu,
+    InsertionLocation methodDeclarationInClass(
+            const CPlusPlus::TranslationUnit *tu,
             const CPlusPlus::ClassSpecifierAST *clazz,
             AccessSpec xsSpec,
-                                               Position positionInAccessSpec = AccessSpecEnd) const;
+            Position positionInAccessSpec = AccessSpecEnd,
+            ForceAccessSpec forceAccessSpec = ForceAccessSpec::No
+            ) const;
 
     InsertionLocation constructorDeclarationInClass(const CPlusPlus::TranslationUnit *tu,
                                                     const CPlusPlus::ClassSpecifierAST *clazz,