Commit aafbf2ea authored by Nikolai Kosjar's avatar Nikolai Kosjar

C++: Make pointer declaration formatter more robust

- Abort on expanded tokens
- Abort on simple declarations starting with "class"/"struct"/"enum"
- Abort if rewritten declaration does not contain '*'/'&'

Change-Id: Ifddb6f20d6bc5c0afc3fcd1d742615198515a04c
Reviewed-by: default avatarErik Verbruggen <erik.verbruggen@digia.com>
parent bd7dfeee
...@@ -32,12 +32,24 @@ ...@@ -32,12 +32,24 @@
#include <AST.h> #include <AST.h>
#include <QtGlobal>
#include <QTextCursor> #include <QTextCursor>
#define DEBUG_OUTPUT 0 #define DEBUG_OUTPUT 0
#define CHECK_RV(cond, err, r) if (!(cond)) { if (DEBUG_OUTPUT) qDebug() << (err); return r; }
#define CHECK_R(cond, err) if (!(cond)) { if (DEBUG_OUTPUT) qDebug() << (err); return; } #if DEBUG_OUTPUT
#define CHECK_C(cond, err) if (!(cond)) { if (DEBUG_OUTPUT) qDebug() << (err); continue; } # include <typeinfo>
# ifdef __GNUC__
# include <cxxabi.h>
# endif
#endif
#define CHECK_RV(cond, err, r) \
if (!(cond)) { if (DEBUG_OUTPUT) qDebug() << "Discarded:" << (err); return r; }
#define CHECK_R(cond, err) \
if (!(cond)) { if (DEBUG_OUTPUT) qDebug() << "Discarded:" << (err); return; }
#define CHECK_C(cond, err) \
if (!(cond)) { if (DEBUG_OUTPUT) qDebug() << "Discarded:" << (err); continue; }
using namespace CppTools; using namespace CppTools;
...@@ -122,6 +134,11 @@ PointerDeclarationFormatter::PointerDeclarationFormatter( ...@@ -122,6 +134,11 @@ PointerDeclarationFormatter::PointerDeclarationFormatter(
bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast) bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true); CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
const unsigned tokenKind = tokenAt(ast->firstToken()).kind();
const bool astIsOk = tokenKind != T_CLASS && tokenKind != T_STRUCT && tokenKind != T_ENUM;
CHECK_RV(astIsOk, "Nothing to do for class/struct/enum", true);
DeclaratorListAST *declaratorList = ast->declarator_list; DeclaratorListAST *declaratorList = ast->declarator_list;
CHECK_RV(declaratorList, "No declarator list", true); CHECK_RV(declaratorList, "No declarator list", true);
...@@ -151,7 +168,7 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast) ...@@ -151,7 +168,7 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast)
// Specify activation range // Specify activation range
int lastActivationToken = 0; int lastActivationToken = 0;
Range range; TokenRange range;
// (2) Handle function declaration's return type // (2) Handle function declaration's return type
if (symbol->type()->asFunctionType()) { if (symbol->type()->asFunctionType()) {
PostfixDeclaratorListAST *pfDeclaratorList = declarator->postfix_declarator_list; PostfixDeclaratorListAST *pfDeclaratorList = declarator->postfix_declarator_list;
...@@ -179,7 +196,7 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast) ...@@ -179,7 +196,7 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast)
firstActivationToken = declarator->firstToken(); firstActivationToken = declarator->firstToken();
} }
range.start = m_cppRefactoringFile->startOf(firstActivationToken); range.start = firstActivationToken;
// (1) Handle 'normal' declarations. // (1) Handle 'normal' declarations.
} else { } else {
...@@ -191,15 +208,16 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast) ...@@ -191,15 +208,16 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast)
declarator->firstToken(), declarator->firstToken(),
&foundBegin); &foundBegin);
CHECK_RV(foundBegin, "Declaration without attributes not supported", true); CHECK_RV(foundBegin, "Declaration without attributes not supported", true);
range.start = m_cppRefactoringFile->startOf(firstActivationToken); range.start = firstActivationToken;
} else { } else {
range.start = m_cppRefactoringFile->startOf(declarator); range.start = declarator->firstToken();
} }
lastActivationToken = declarator->equal_token lastActivationToken = declarator->equal_token
? declarator->equal_token - 1 ? declarator->equal_token - 1
: declarator->lastToken() - 1; : declarator->lastToken() - 1;
} }
range.end = m_cppRefactoringFile->endOf(lastActivationToken);
range.end = lastActivationToken;
checkAndRewrite(symbol, range, charactersToRemove); checkAndRewrite(symbol, range, charactersToRemove);
} }
...@@ -209,6 +227,9 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast) ...@@ -209,6 +227,9 @@ bool PointerDeclarationFormatter::visit(SimpleDeclarationAST *ast)
/*! Handle return types of function definitions */ /*! Handle return types of function definitions */
bool PointerDeclarationFormatter::visit(FunctionDefinitionAST *ast) bool PointerDeclarationFormatter::visit(FunctionDefinitionAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
DeclaratorAST *declarator = ast->declarator; DeclaratorAST *declarator = ast->declarator;
CHECK_RV(declarator, "No declarator", true); CHECK_RV(declarator, "No declarator", true);
CHECK_RV(declarator->ptr_operator_list, "No Pointer or references", true); CHECK_RV(declarator->ptr_operator_list, "No Pointer or references", true);
...@@ -230,8 +251,7 @@ bool PointerDeclarationFormatter::visit(FunctionDefinitionAST *ast) ...@@ -230,8 +251,7 @@ bool PointerDeclarationFormatter::visit(FunctionDefinitionAST *ast)
lastActivationToken, lastActivationToken,
&foundBegin); &foundBegin);
CHECK_RV(foundBegin, "Declaration without attributes not supported", true); CHECK_RV(foundBegin, "Declaration without attributes not supported", true);
Range range(m_cppRefactoringFile->startOf(firstActivationToken), TokenRange range(firstActivationToken, lastActivationToken);
m_cppRefactoringFile->endOf(lastActivationToken));
checkAndRewrite(symbol, range); checkAndRewrite(symbol, range);
return true; return true;
...@@ -240,6 +260,9 @@ bool PointerDeclarationFormatter::visit(FunctionDefinitionAST *ast) ...@@ -240,6 +260,9 @@ bool PointerDeclarationFormatter::visit(FunctionDefinitionAST *ast)
/*! Handle parameters in function declarations and definitions */ /*! Handle parameters in function declarations and definitions */
bool PointerDeclarationFormatter::visit(ParameterDeclarationAST *ast) bool PointerDeclarationFormatter::visit(ParameterDeclarationAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
DeclaratorAST *declarator = ast->declarator; DeclaratorAST *declarator = ast->declarator;
CHECK_RV(declarator, "No declarator", true); CHECK_RV(declarator, "No declarator", true);
CHECK_RV(declarator->ptr_operator_list, "No Pointer or references", true); CHECK_RV(declarator->ptr_operator_list, "No Pointer or references", true);
...@@ -249,8 +272,7 @@ bool PointerDeclarationFormatter::visit(ParameterDeclarationAST *ast) ...@@ -249,8 +272,7 @@ bool PointerDeclarationFormatter::visit(ParameterDeclarationAST *ast)
const int lastActivationToken = ast->equal_token const int lastActivationToken = ast->equal_token
? ast->equal_token - 1 ? ast->equal_token - 1
: ast->lastToken() - 1; : ast->lastToken() - 1;
Range range(m_cppRefactoringFile->startOf(ast), TokenRange range(ast->firstToken(), lastActivationToken);
m_cppRefactoringFile->endOf(lastActivationToken));
checkAndRewrite(symbol, range); checkAndRewrite(symbol, range);
return true; return true;
...@@ -259,6 +281,9 @@ bool PointerDeclarationFormatter::visit(ParameterDeclarationAST *ast) ...@@ -259,6 +281,9 @@ bool PointerDeclarationFormatter::visit(ParameterDeclarationAST *ast)
/*! Handle declaration in foreach statement */ /*! Handle declaration in foreach statement */
bool PointerDeclarationFormatter::visit(ForeachStatementAST *ast) bool PointerDeclarationFormatter::visit(ForeachStatementAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
DeclaratorAST *declarator = ast->declarator; DeclaratorAST *declarator = ast->declarator;
CHECK_RV(declarator, "No declarator", true); CHECK_RV(declarator, "No declarator", true);
CHECK_RV(declarator->ptr_operator_list, "No Pointer or references", true); CHECK_RV(declarator->ptr_operator_list, "No Pointer or references", true);
...@@ -272,8 +297,7 @@ bool PointerDeclarationFormatter::visit(ForeachStatementAST *ast) ...@@ -272,8 +297,7 @@ bool PointerDeclarationFormatter::visit(ForeachStatementAST *ast)
const int lastActivationToken = declarator->equal_token const int lastActivationToken = declarator->equal_token
? declarator->equal_token - 1 ? declarator->equal_token - 1
: declarator->lastToken() - 1; : declarator->lastToken() - 1;
Range range(m_cppRefactoringFile->startOf(firstSpecifier), TokenRange range(firstSpecifier->firstToken(), lastActivationToken);
m_cppRefactoringFile->endOf(lastActivationToken));
checkAndRewrite(symbol, range); checkAndRewrite(symbol, range);
return true; return true;
...@@ -281,18 +305,24 @@ bool PointerDeclarationFormatter::visit(ForeachStatementAST *ast) ...@@ -281,18 +305,24 @@ bool PointerDeclarationFormatter::visit(ForeachStatementAST *ast)
bool PointerDeclarationFormatter::visit(IfStatementAST *ast) bool PointerDeclarationFormatter::visit(IfStatementAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
processIfWhileForStatement(ast->condition, ast->symbol); processIfWhileForStatement(ast->condition, ast->symbol);
return true; return true;
} }
bool PointerDeclarationFormatter::visit(WhileStatementAST *ast) bool PointerDeclarationFormatter::visit(WhileStatementAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
processIfWhileForStatement(ast->condition, ast->symbol); processIfWhileForStatement(ast->condition, ast->symbol);
return true; return true;
} }
bool PointerDeclarationFormatter::visit(ForStatementAST *ast) bool PointerDeclarationFormatter::visit(ForStatementAST *ast)
{ {
CHECK_RV(ast, "Invalid AST", true);
printCandidate(ast);
processIfWhileForStatement(ast->condition, ast->symbol); processIfWhileForStatement(ast->condition, ast->symbol);
return true; return true;
} }
...@@ -330,8 +360,7 @@ void PointerDeclarationFormatter::processIfWhileForStatement(ExpressionAST *expr ...@@ -330,8 +360,7 @@ void PointerDeclarationFormatter::processIfWhileForStatement(ExpressionAST *expr
} }
// Specify activation range // Specify activation range
Range range(m_cppRefactoringFile->startOf(condition), TokenRange range(condition->firstToken(), declarator->equal_token - 1);
m_cppRefactoringFile->endOf(declarator->equal_token - 1));
checkAndRewrite(symbol, range); checkAndRewrite(symbol, range);
} }
...@@ -343,13 +372,23 @@ void PointerDeclarationFormatter::processIfWhileForStatement(ExpressionAST *expr ...@@ -343,13 +372,23 @@ void PointerDeclarationFormatter::processIfWhileForStatement(ExpressionAST *expr
\param symbol the symbol to be rewritten \param symbol the symbol to be rewritten
\param range the substitution range in the file \param range the substitution range in the file
*/ */
void PointerDeclarationFormatter::checkAndRewrite(Symbol *symbol, Range range, void PointerDeclarationFormatter::checkAndRewrite(Symbol *symbol, TokenRange tokenRange,
unsigned charactersToRemove) unsigned charactersToRemove)
{ {
CHECK_R(range.start >= 0 && range.end > 0, "Range invalid"); CHECK_R(tokenRange.end > 0, "TokenRange invalid1");
CHECK_R(range.start < range.end, "Range invalid"); CHECK_R(tokenRange.start < tokenRange.end, "TokenRange invalid2");
CHECK_R(symbol, "No symbol"); CHECK_R(symbol, "No symbol");
// Check for expanded tokens
for (unsigned token = tokenRange.start; token <= tokenRange.end; ++token)
CHECK_R(! tokenAt(token).expanded(), "Token is expanded");
Range range(m_cppRefactoringFile->startOf(tokenRange.start),
m_cppRefactoringFile->endOf(tokenRange.end));
CHECK_R(range.start >= 0 && range.end > 0, "ChangeRange invalid1");
CHECK_R(range.start < range.end, "ChangeRange invalid2");
// Check range with respect to cursor position / selection // Check range with respect to cursor position / selection
if (m_cursorHandling == RespectCursor) { if (m_cursorHandling == RespectCursor) {
const QTextCursor cursor = m_cppRefactoringFile->cursor(); const QTextCursor cursor = m_cppRefactoringFile->cursor();
...@@ -374,10 +413,14 @@ void PointerDeclarationFormatter::checkAndRewrite(Symbol *symbol, Range range, ...@@ -374,10 +413,14 @@ void PointerDeclarationFormatter::checkAndRewrite(Symbol *symbol, Range range,
// Does the rewritten declaration (part) differs from the original source (part)? // Does the rewritten declaration (part) differs from the original source (part)?
QString rewrittenDeclaration = rewriteDeclaration(type, symbol->name()); QString rewrittenDeclaration = rewriteDeclaration(type, symbol->name());
rewrittenDeclaration.remove(0, charactersToRemove); rewrittenDeclaration.remove(0, charactersToRemove);
CHECK_R(originalDeclaration != rewrittenDeclaration, "Rewritten is same as original"); CHECK_R(originalDeclaration != rewrittenDeclaration, "Rewritten is same as original");
CHECK_R(rewrittenDeclaration.contains(QLatin1Char('&'))
|| rewrittenDeclaration.contains(QLatin1Char('*')),
"No pointer or references in rewritten declaration");
if (DEBUG_OUTPUT) { if (DEBUG_OUTPUT) {
qDebug("Rewritten: \"%s\" --> \"%s\"", originalDeclaration.toLatin1().constData(), qDebug("==> Rewritten: \"%s\" --> \"%s\"", originalDeclaration.toLatin1().constData(),
rewrittenDeclaration.toLatin1().constData()); rewrittenDeclaration.toLatin1().constData());
} }
...@@ -415,3 +458,22 @@ QString PointerDeclarationFormatter::rewriteDeclaration(FullySpecifiedType type, ...@@ -415,3 +458,22 @@ QString PointerDeclarationFormatter::rewriteDeclaration(FullySpecifiedType type,
return m_overview.prettyType(type, QLatin1String(identifier)); return m_overview.prettyType(type, QLatin1String(identifier));
} }
void PointerDeclarationFormatter::printCandidate(AST *ast)
{
#if DEBUG_OUTPUT
QString tokens;
for (unsigned token = ast->firstToken(); token < ast->lastToken(); token++)
tokens += QString::fromLatin1(tokenAt(token).spell()) + QLatin1Char(' ');
# ifdef __GNUC__
QByteArray name = abi::__cxa_demangle(typeid(*ast).name(), 0, 0, 0) + 11;
name.truncate(name.length() - 3);
# else
QByteArray name = typeid(*ast).name();
# endif
qDebug("--> Candidate: %s: %s", name.constData(), qPrintable(tokens));
#else
Q_UNUSED(ast)
#endif // DEBUG_OUTPUT
}
...@@ -106,9 +106,18 @@ protected: ...@@ -106,9 +106,18 @@ protected:
bool visit(ForeachStatementAST *ast); bool visit(ForeachStatementAST *ast);
private: private:
class TokenRange {
public:
TokenRange() : start(0), end(0) {}
TokenRange(unsigned start, unsigned end) : start(start), end(end) {}
unsigned start;
unsigned end;
};
void processIfWhileForStatement(ExpressionAST *expression, Symbol *symbol); void processIfWhileForStatement(ExpressionAST *expression, Symbol *symbol);
void checkAndRewrite(Symbol *symbol, Range range, unsigned charactersToRemove = 0); void checkAndRewrite(Symbol *symbol, TokenRange range, unsigned charactersToRemove = 0);
QString rewriteDeclaration(FullySpecifiedType type, const Name *name) const; QString rewriteDeclaration(FullySpecifiedType type, const Name *name) const;
void printCandidate(AST *ast);
const CppRefactoringFilePtr m_cppRefactoringFile; const CppRefactoringFilePtr m_cppRefactoringFile;
const Overview &m_overview; const Overview &m_overview;
......
...@@ -575,3 +575,113 @@ void CppToolsPlugin::test_format_pointerdeclaration_multiple_matches_data() ...@@ -575,3 +575,113 @@ void CppToolsPlugin::test_format_pointerdeclaration_multiple_matches_data()
" return 0;\n" " return 0;\n"
"}\n"; "}\n";
} }
void CppToolsPlugin::test_format_pointerdeclaration_macros()
{
QFETCH(QString, source);
QFETCH(QString, reformattedSource);
TestEnvironment env(source.toLatin1(), Document::ParseTranlationUnit);
AST *ast = env.translationUnit->ast();
QVERIFY(ast);
env.applyFormatting(ast, PointerDeclarationFormatter::RespectCursor);
QCOMPARE(env.textDocument->toPlainText(), reformattedSource);
}
void CppToolsPlugin::test_format_pointerdeclaration_macros_data()
{
QTest::addColumn<QString>("source");
QTest::addColumn<QString>("reformattedSource");
QString source;
source = QLatin1String(
"#define FOO int*\n"
"FOO @bla;\n");
QTest::newRow("macro-in-simple-declaration")
<< source << stripCursor(source);
source = QLatin1String(
"#define FOO int*\n"
"FOO @f();\n");
QTest::newRow("macro-in-function-declaration-returntype")
<< source << stripCursor(source);
source = QLatin1String(
"#define FOO int*\n"
"int f(@FOO a);\n");
QTest::newRow("macro-in-function-declaration-param")
<< source << stripCursor(source);
source = QLatin1String(
"#define FOO int*\n"
"FOO @f() {};\n");
QTest::newRow("macro-in-function-definition-returntype")
<< source << stripCursor(source);
source = QLatin1String(
"#define FOO int*\n"
"int f(FOO @a) {};\n");
QTest::newRow("macro-in-function-definition-param")
<< source << stripCursor(source);
source = QLatin1String(
"#define FOO int*\n"
"while (FOO @s = 0);\n");
QTest::newRow("macro-in-if-while-for")
<< source << stripCursor(source);
source = QLatin1String(
"#define FOO int*\n"
"foreach (FOO @s, list);\n");
QTest::newRow("macro-in-foreach")
<< source << stripCursor(source);
// The bug was that we got "Reformat to 'QMetaObject staticMetaObject'"
// at the cursor position below, which was completelty wrong.
QTest::newRow("wrong-reformat-suggestion")
<<
"#define Q_OBJECT \\\n"
"public: \\\n"
" template <typename T> inline void qt_check_for_QOBJECT_macro(T &_q_argument) \\\n"
" { int i = qYouForgotTheQ_OBJECT_Macro(this, &_q_argument); i = i; } \\\n"
" QMetaObject staticMetaObject; \\\n"
" void *qt_metacast(const char *); \\\n"
" static inline QString tr(const char *s, const char *f = 0); \\\n"
" \n"
"class KitInformation\n"
"{\n"
" Q_OBJECT\n"
"public:\n"
" typedef QPair<QString, QString> Item;\n"
" \n"
" Core::Id dataId(); // the higher the closer to top.\n"
" \n"
" unsigned int priority() = 0;\n"
" \n"
" QVariant defaultValue(Kit@*) = 0;\n"
"};\n"
<<
"#define Q_OBJECT \\\n"
"public: \\\n"
" template <typename T> inline void qt_check_for_QOBJECT_macro(T &_q_argument) \\\n"
" { int i = qYouForgotTheQ_OBJECT_Macro(this, &_q_argument); i = i; } \\\n"
" QMetaObject staticMetaObject; \\\n"
" void *qt_metacast(const char *); \\\n"
" static inline QString tr(const char *s, const char *f = 0); \\\n"
" \n"
"class KitInformation\n"
"{\n"
" Q_OBJECT\n"
"public:\n"
" typedef QPair<QString, QString> Item;\n"
" \n"
" Core::Id dataId(); // the higher the closer to top.\n"
" \n"
" unsigned int priority() = 0;\n"
" \n"
" QVariant defaultValue(Kit *) = 0;\n"
"};\n";
}
...@@ -130,6 +130,8 @@ private slots: ...@@ -130,6 +130,8 @@ private slots:
void test_format_pointerdeclaration_multiple_declarators_data(); void test_format_pointerdeclaration_multiple_declarators_data();
void test_format_pointerdeclaration_multiple_matches(); void test_format_pointerdeclaration_multiple_matches();
void test_format_pointerdeclaration_multiple_matches_data(); void test_format_pointerdeclaration_multiple_matches_data();
void test_format_pointerdeclaration_macros();
void test_format_pointerdeclaration_macros_data();
void test_modelmanager_paths(); void test_modelmanager_paths();
void test_modelmanager_framework_headers(); void test_modelmanager_framework_headers();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment