Commit f10b978f authored by Nikolai Kosjar's avatar Nikolai Kosjar

C++: Improve GetterSetter quick fix

- Prefix getter name with 'get' if there is a conflict between
  the getter name and the member variable name.
- When possible, use base name of member variable as setter parameter
  name instead of 'value'.
- Generate static getters/setters for static members.
- Fix case "class C { char *@s; };" - the quick fix was not offered for
  this particular cursor position (right after pointer sign).
- Fix case "class C { char c, *@s; };" - the quick fix was done for the
  wrong type (char instead of char *).
- Do not generate a setter for const member variables.
- Do not get triggered on member functions and arrays.
- Do not offer the quick fix if there is already a member with the
  getter or setter name we would generate.

Change-Id: I4530467518ea0bf6368e47eb32d5faafbf8cd928
Reviewed-by: default avatarErik Verbruggen <erik.verbruggen@digia.com>
parent dc5a2d6b
......@@ -322,18 +322,20 @@ namespace {
class GetterSetterOperation : public CppQuickFixOperation
{
public:
GetterSetterOperation(const QSharedPointer<const CppQuickFixAssistInterface> &interface)
GetterSetterOperation(const QSharedPointer<const CppQuickFixAssistInterface> &interface,
bool testMode = false)
: CppQuickFixOperation(interface)
, m_variableName(0)
, m_declaratorId(0)
, m_declarator(0)
, m_variableDecl(0)
, m_classSpecifier(0)
, m_classDecl(0)
, m_offerQuickFix(true)
, m_testMode(testMode)
{
setDescription(TextEditor::QuickFixFactory::tr("Create Getter and Setter Member Functions"));
m_variableName = 0;
m_declaratorId = 0;
m_declarator = 0;
m_variableDecl = 0;
m_classSpecifier = 0;
m_classDecl = 0;
const QList<AST *> &path = interface->path();
// We expect something like
// [0] TranslationUnitAST
......@@ -350,12 +352,79 @@ public:
if (n < 6)
return;
m_variableName = path.at(n - 1)->asSimpleName();
m_declaratorId = path.at(n - 2)->asDeclaratorId();
m_declarator = path.at(n - 3)->asDeclarator();
m_variableDecl = path.at(n - 4)->asSimpleDeclaration();
m_classSpecifier = path.at(n - 5)->asClassSpecifier();
m_classDecl = path.at(n - 6)->asSimpleDeclaration();
int i = 1;
m_variableName = path.at(n - i++)->asSimpleName();
m_declaratorId = path.at(n - i++)->asDeclaratorId();
// DeclaratorAST might be preceded by PointerAST, e.g. for the case
// "class C { char *@s; };", where '@' denotes the text cursor position.
if (!(m_declarator = path.at(n - i++)->asDeclarator())) {
--i;
if (path.at(n - i++)->asPointer()) {
if (n < 7)
return;
m_declarator = path.at(n - i++)->asDeclarator();
}
}
m_variableDecl = path.at(n - i++)->asSimpleDeclaration();
m_classSpecifier = path.at(n - i++)->asClassSpecifier();
m_classDecl = path.at(n - i++)->asSimpleDeclaration();
if (!isValid())
return;
// Do not get triggered on member functions and arrays
if (m_declarator->postfix_declarator_list) {
m_offerQuickFix = false;
return;
}
// Construct getter and setter names
const Name *variableName = m_variableName->name;
if (!variableName) {
m_offerQuickFix = false;
return;
}
const Identifier *variableId = variableName->identifier();
if (!variableId) {
m_offerQuickFix = false;
return;
}
m_variableString = QString::fromLatin1(variableId->chars(), variableId->size());
m_baseName = m_variableString;
if (m_baseName.startsWith(QLatin1String("m_")))
m_baseName.remove(0, 2);
else if (m_baseName.startsWith(QLatin1Char('_')))
m_baseName.remove(0, 1);
else if (m_baseName.endsWith(QLatin1Char('_')))
m_baseName.chop(1);
m_getterName = m_baseName != m_variableString
? QString::fromLatin1("%1").arg(m_baseName)
: QString::fromLatin1("get%1%2")
.arg(m_baseName.left(1).toUpper()).arg(m_baseName.mid(1));
m_setterName = QString::fromLatin1("set%1%2")
.arg(m_baseName.left(1).toUpper()).arg(m_baseName.mid(1));
// Check if the class has already a getter or setter.
// This is only a simple check which should suffice not triggering the
// same quick fix again. Limitations:
// 1) It only checks in the current class, but not in base classes.
// 2) It compares only names instead of types/signatures.
if (Class *klass = m_classSpecifier->symbol) {
for (unsigned i = 0; i < klass->memberCount(); ++i) {
Symbol *symbol = klass->memberAt(i);
if (const Name *symbolName = symbol->name()) {
if (const Identifier *id = symbolName->identifier()) {
const QString memberName = QString::fromLatin1(id->chars(), id->size());
if (memberName == m_getterName || memberName == m_setterName) {
m_offerQuickFix = false;
return;
}
}
}
} // for
}
}
bool isValid() const
......@@ -365,7 +434,8 @@ public:
&& m_declarator
&& m_variableDecl
&& m_classSpecifier
&& m_classDecl;
&& m_classDecl
&& m_offerQuickFix;
}
void perform()
......@@ -373,24 +443,35 @@ public:
CppRefactoringChanges refactoring(snapshot());
CppRefactoringFilePtr currentFile = refactoring.file(fileName());
const Name *variableName = m_variableName->name;
QTC_ASSERT(variableName, return);
const Identifier *variableId = variableName->identifier();
QTC_ASSERT(variableId, return);
QString variableString = QString::fromLatin1(variableId->chars(), variableId->size());
const List<Symbol *> *symbols = m_variableDecl->symbols;
QTC_ASSERT(symbols, return);
Symbol *symbol = symbols->value;
// Find the right symbol in the simple declaration
Symbol *symbol = 0;
for (; symbols; symbols = symbols->next) {
Symbol *s = symbols->value;
if (const Name *name = s->name()) {
if (const Identifier *id = name->identifier()) {
const QString symbolName = QString::fromLatin1(id->chars(), id->size());
if (symbolName == m_variableString) {
symbol = s;
break;
}
}
}
}
QTC_ASSERT(symbol, return);
FullySpecifiedType fullySpecifiedType = symbol->type();
Type *type = fullySpecifiedType.type();
QTC_ASSERT(type, return);
Overview oo;
if (!m_testMode)
oo = CppCodeStyleSettings::currentProjectCodeStyleOverview();
oo.showFunctionSignatures = true;
oo.showReturnTypes = true;
oo.showArgumentNames = true;
QString typeString = oo.prettyType(fullySpecifiedType);
const QString typeString = oo.prettyType(fullySpecifiedType);
const NameAST *classNameAST = m_classSpecifier->name;
QTC_ASSERT(classNameAST, return);
......@@ -411,45 +492,72 @@ public:
InsertionLocation declLocation = locator.methodDeclarationInClass
(declFileName, m_classSpecifier->symbol->asClass(), InsertionPointLocator::Public);
QString baseName = variableString;
if (baseName.startsWith(QLatin1String("m_")))
baseName.remove(0, 2);
else if (baseName.startsWith(QLatin1Char('_')))
baseName.remove(0, 1);
else if (baseName.endsWith(QLatin1Char('_')))
baseName.chop(1);
QString getterName = QString::fromLatin1("%1").arg(baseName);
QString setterName = QString::fromLatin1("set%1%2")
.arg(baseName.left(1).toUpper()).arg(baseName.mid(1));
const bool passByValue = type->isIntegerType() || type->isFloatType()
|| type->isPointerType() || type->isEnumType();
const char *param = passByValue ? "%1" : "const %1 &";
QString paramString = QString::fromLatin1(param).arg(typeString);
const QString paramName = m_baseName != m_variableString
? m_baseName : QLatin1String("value");
QString paramString;
if (passByValue) {
paramString = oo.prettyType(fullySpecifiedType, paramName);
} else {
FullySpecifiedType constParamType(fullySpecifiedType);
constParamType.setConst(true);
QScopedPointer<ReferenceType> referenceType(new ReferenceType(constParamType, false));
FullySpecifiedType referenceToConstParamType(referenceType.data());
paramString = oo.prettyType(referenceToConstParamType, paramName);
}
const bool isStatic = symbol->storage() == Symbol::Static;
// Construct declaration strings
QString declaration = declLocation.prefix();
declaration += QString::fromLatin1(
"%3 %1() const;\n" "void %2(%4 value);\n")
.arg(getterName).arg(setterName)
.arg(typeString).arg(paramString);
declaration += declLocation.suffix();
QString getterTypeString = typeString;
FullySpecifiedType getterType(fullySpecifiedType);
if (fullySpecifiedType.isConst()) {
getterType.setConst(false);
getterTypeString = oo.prettyType(getterType);
}
QString implementation = QString::fromLatin1(
"\n%5 %3::%1() const\n"
"{\n"
"return %4;\n"
"}\n"
"\nvoid %3::%2(%6 value)\n"
"{\n"
"%4 = value;\n"
"}\n")
.arg(getterName).arg(setterName)
.arg(classString).arg(variableString)
.arg(typeString).arg(paramString);
const QString declarationGetterTypeAndNameString = oo.prettyType(getterType, m_getterName);
const QString declarationGetter = QString::fromLatin1("%1%2()%3;\n")
.arg(isStatic ? QLatin1String("static ") : QString())
.arg(declarationGetterTypeAndNameString)
.arg(isStatic ? QString() : QLatin1String(" const"));
const QString declarationSetter = QString::fromLatin1("%1void %2(%3);\n")
.arg(isStatic ? QLatin1String("static ") : QString())
.arg(m_setterName)
.arg(paramString);
declaration += declarationGetter;
if (!fullySpecifiedType.isConst())
declaration += declarationSetter;
declaration += declLocation.suffix();
// Construct implementation strings
const QString implementationGetterTypeAndNameString = oo.prettyType(
getterType, QString::fromLatin1("%1::%2").arg(classString, m_getterName));
const QString implementationGetter = QString::fromLatin1(
"\n%1()%2\n"
"{\n"
"return %3;\n"
"}\n")
.arg(implementationGetterTypeAndNameString)
.arg(isStatic ? QString() : QLatin1String(" const"))
.arg(m_variableString);
const QString implementationSetter = QString::fromLatin1(
"\nvoid %1::%2(%3)\n"
"{\n"
"%4 = %5;\n"
"}\n")
.arg(classString).arg(m_setterName)
.arg(paramString).arg(m_variableString)
.arg(paramName);
QString implementation = implementationGetter;
if (!fullySpecifiedType.isConst())
implementation += implementationSetter;
// Create and apply changes
ChangeSet currChanges;
int declInsertPos = currentFile->position(qMax(1u, declLocation.line()),
declLocation.column());
currChanges.insert(declInsertPos, declaration);
......@@ -483,13 +591,20 @@ public:
SimpleDeclarationAST *m_variableDecl;
ClassSpecifierAST *m_classSpecifier;
SimpleDeclarationAST *m_classDecl;
QString m_baseName;
QString m_getterName;
QString m_setterName;
QString m_variableString;
bool m_offerQuickFix;
bool m_testMode;
};
} // namespace
void GetterSetter::match(const CppQuickFixInterface &interface, QuickFixOperations &result)
{
GetterSetterOperation *op = new GetterSetterOperation(interface);
GetterSetterOperation *op = new GetterSetterOperation(interface, m_testMode);
if (op->isValid())
result.append(CppQuickFixOperation::Ptr(op));
else
......
......@@ -56,7 +56,10 @@ public:
class GetterSetter : public CppQuickFixFactory
{
public:
GetterSetter(const bool testMode = false) : m_testMode(testMode) {}
void match(const CppQuickFixInterface &interface, TextEditor::QuickFixOperations &result);
private:
const bool m_testMode;
};
} // namespace Internal
......
......@@ -89,7 +89,18 @@ private slots:
#ifdef WITH_TESTS
private slots: // quickfix tests
void test_quickfix_GetterSetter();
void test_quickfix_GetterSetter_basicGetterWithPrefix();
void test_quickfix_GetterSetter_basicGetterWithoutPrefix();
void test_quickfix_GetterSetter_customType();
void test_quickfix_GetterSetter_constMember();
void test_quickfix_GetterSetter_pointerToNonConst();
void test_quickfix_GetterSetter_pointerToConst();
void test_quickfix_GetterSetter_staticMember();
void test_quickfix_GetterSetter_secondDeclarator();
void test_quickfix_GetterSetter_triggeringRightAfterPointerSign();
void test_quickfix_GetterSetter_notTriggeringOnMemberFunction();
void test_quickfix_GetterSetter_notTriggeringOnMemberArray();
void test_quickfix_GetterSetter_notTriggeringWhenGetterOrSetterExist();
#endif // WITH_TESTS
private:
......
......@@ -28,33 +28,35 @@
****************************************************************************/
#include <AST.h>
#include <Bind.h>
#include <Control.h>
#include <CppDocument.h>
#include <DiagnosticClient.h>
#include <Scope.h>
#include <TranslationUnit.h>
#include <Literals.h>
#include <Bind.h>
#include <Scope.h>
#include <Symbols.h>
#include <utils/changeset.h>
#include <texteditor/basetextdocument.h>
#include <texteditor/plaintexteditor.h>
#include <texteditor/codeassist/iassistproposal.h>
#include <texteditor/codeassist/iassistproposalmodel.h>
#include <texteditor/codeassist/basicproposalitemlistmodel.h>
#include <TranslationUnit.h>
#include <coreplugin/editormanager/editormanager.h>
#include <cppeditor/cppeditor.h>
#include <cppeditor/cppinsertdecldef.h>
#include <cppeditor/cppplugin.h>
#include <cppeditor/cppquickfix.h>
#include <cppeditor/cppquickfixassistant.h>
#include <cppeditor/cppinsertdecldef.h>
#include <cppeditor/cppquickfix.h>
#include <extensionsystem/pluginmanager.h>
#include <texteditor/basetextdocument.h>
#include <texteditor/codeassist/basicproposalitemlistmodel.h>
#include <texteditor/codeassist/iassistproposal.h>
#include <texteditor/codeassist/iassistproposalmodel.h>
#include <texteditor/plaintexteditor.h>
#include <utils/changeset.h>
#include <utils/fileutils.h>
#include <QtTest>
#include <QDebug>
#include <QTextDocument>
#include <QDir>
#include <QTextDocument>
#include <QtTest>
/*!
Tests for quick-fixes.
......@@ -83,21 +85,21 @@ struct TestCase
QuickFixOperation::Ptr getFix(CppQuickFixFactory *factory);
void run(CppQuickFixFactory *factory, const QByteArray &expected, int undoCount = 1);
void run(CppQuickFixFactory *factory, const QByteArray &expected, bool changesExpected = true,
int undoCount = 1);
private:
TestCase(const TestCase &);
TestCase &operator=(const TestCase &);
};
/// apply the factory on the source, and get back the first result.
/// Apply the factory on the source and get back the first result or a null pointer.
QuickFixOperation::Ptr TestCase::getFix(CppQuickFixFactory *factory)
{
CppQuickFixInterface qfi(new CppQuickFixAssistInterface(editorWidget, ExplicitlyInvoked));
TextEditor::QuickFixOperations results;
factory->match(qfi, results);
Q_ASSERT(!results.isEmpty());
return results.first();
return results.isEmpty() ? QuickFixOperation::Ptr() : results.first();
}
/// The '@' in the input is the position from where the quick-fix discovery is triggered.
......@@ -165,14 +167,20 @@ QByteArray &removeTrailingWhitespace(QByteArray &input)
return input;
}
void TestCase::run(CppQuickFixFactory *factory, const QByteArray &expected, int undoCount)
void TestCase::run(CppQuickFixFactory *factory, const QByteArray &expected,
bool changesExpected, int undoCount)
{
QuickFixOperation::Ptr fix = getFix(factory);
if (!fix) {
QVERIFY2(!changesExpected, "No QuickFixOperation");
return;
}
fix->perform();
QByteArray result = editorWidget->document()->toPlainText().toUtf8();
removeTrailingWhitespace(result);
QCOMPARE(result, expected);
QCOMPARE(QLatin1String(result), QLatin1String(expected));
for (int i = 0; i < undoCount; ++i)
editorWidget->undo();
......@@ -182,7 +190,11 @@ void TestCase::run(CppQuickFixFactory *factory, const QByteArray &expected, int
}
} // anonymous namespace
void CppPlugin::test_quickfix_GetterSetter()
/// Checks:
/// 1. If the name does not start with ("m_" or "_") and does not
/// end with "_", we are forced to prefix the getter with "get".
/// 2. Setter: Use pass by value on integer/float and pointer types.
void CppPlugin::test_quickfix_GetterSetter_basicGetterWithPrefix()
{
TestCase data("\n"
"class Something\n"
......@@ -196,12 +208,258 @@ void CppPlugin::test_quickfix_GetterSetter()
" int it;\n"
"\n"
"public:\n"
" int it() const;\n"
" int getIt() const;\n"
" void setIt(int value);\n"
"};\n"
"\n"
"int Something::getIt() const\n"
"{\n"
" return it;\n"
"}\n"
"\n"
"void Something::setIt(int value)\n"
"{\n"
" it = value;\n"
"}\n"
"\n"
;
GetterSetter factory(/*testMode=*/ true);
data.run(&factory, expected);
}
/// Checks:
/// 1. Getter: "get" prefix is not necessary.
/// 2. Setter: Parameter name is base name.
void CppPlugin::test_quickfix_GetterSetter_basicGetterWithoutPrefix()
{
TestCase data("\n"
"class Something\n"
"{\n"
" int @m_it;\n"
"};\n"
);
QByteArray expected = "\n"
"class Something\n"
"{\n"
" int m_it;\n"
"\n"
"public:\n"
" int it() const;\n"
" void setIt(int it);\n"
"};\n"
"\n"
"int Something::it() const\n"
"{\n"
" return m_it;\n"
"}\n"
"\n"
"void Something::setIt(int it)\n"
"{\n"
" m_it = it;\n"
"}\n"
"\n"
;
GetterSetter factory(/*testMode=*/ true);
data.run(&factory, expected);
}
/// Check: Setter: Use pass by reference for parameters which
/// are not integer, float or pointers.
void CppPlugin::test_quickfix_GetterSetter_customType()
{
TestCase data("\n"
"class Something\n"
"{\n"
" MyType @it;\n"
"};\n"
);
QByteArray expected = "\n"
"class Something\n"
"{\n"
" MyType it;\n"
"\n"
"public:\n"
" MyType getIt() const;\n"
" void setIt(const MyType &value);\n"
"};\n"
"\n"
"MyType Something::getIt() const\n"
"{\n"
" return it;\n"
"}\n"
"\n"
"void Something::setIt(const MyType &value)\n"
"{\n"
" it = value;\n"
"}\n"
"\n"
;
GetterSetter factory(/*testMode=*/ true);
data.run(&factory, expected);
}
/// Checks:
/// 1. Setter: No setter is generated for const members.
/// 2. Getter: Return a non-const type since it pass by value anyway.
void CppPlugin::test_quickfix_GetterSetter_constMember()
{
TestCase data("\n"
"class Something\n"
"{\n"
" const int @it;\n"
"};\n"
);
QByteArray expected = "\n"
"class Something\n"
"{\n"
" const int it;\n"
"\n"
"public:\n"
" int getIt() const;\n"
"};\n"
"\n"
"int Something::getIt() const\n"
"{\n"
" return it;\n"
"}\n"
"\n"
;
GetterSetter factory(/*testMode=*/ true);
data.run(&factory, expected);
}
/// Checks: No special treatment for pointer to non const.
void CppPlugin::test_quickfix_GetterSetter_pointerToNonConst()
{
TestCase data("\n"
"class Something\n"
"{\n"
" int *it@;\n"
"};\n"
);
QByteArray expected = "\n"
"class Something\n"
"{\n"
" int *it;\n"
"\n"
"public:\n"
" int *getIt() const;\n"
" void setIt(int *value);\n"
"};\n"
"\n"
"int *Something::getIt() const\n"
"{\n"
" return it;\n"
"}\n"
"\n"
"void Something::setIt(int *value)\n"
"{\n"
" it = value;\n"
"}\n"
"\n"
;
GetterSetter factory(/*testMode=*/ true);
data.run(&factory, expected);
}
/// Checks: No special treatment for pointer to const.
void CppPlugin::test_quickfix_GetterSetter_pointerToConst()
{
TestCase data("\n"
"class Something\n"
"{\n"
" const int *it@;\n"
"};\n"
);
QByteArray expected = "\n"
"class Something\n"
"{\n"
" const int *it;\n"
"\n"
"public:\n"
" const int *getIt() const;\n"
" void setIt(const int *value);\n"
"};\n"
"\n"
"const int *Something::getIt() const\n"
"{\n"
" return it;\n"
"}\n"
"\n"
"void Something::setIt(const int *value)\n"
"{\n"
" it = value;\n"
"}\n"
"\n"
;
GetterSetter factory(/*testMode=*/ true);
data.run(&factory, expected);
}
/// Checks:
/// 1. Setter: Setter is a static function.
/// 2. Getter: Getter is a static, non const function.
void CppPlugin::test_quickfix_GetterSetter_staticMember()
{
TestCase data("\n"
"class Something\n"
"{\n"
" static int @m_member;\n"
"};\n"
);
QByteArray expected = "\n"
"class Something\n"
"{\n"
" static int m_member;\n"
"\n"
"public:\n"
" static int member();\n"
" static void setMember(int member);\n"
"};\n"
"\n"
"int Something::member()\n"
"{\n"
" return m_member;\n"
"}\n"
"\n"
"void Something::setMember(int member)\n"
"{\n"
" m_member = member;\n"
"}\n"
"\n"
;