From 5ec251a79140995642053f3d27f2cc3fb218e3a9 Mon Sep 17 00:00:00 2001 From: Felix Morgner Date: Wed, 7 Mar 2018 16:14:20 +0100 Subject: [PATCH] Bug 532120: Catch by const reference ignores const placement setting The original implementation used plain-text string manipulation of the IDocument. This changeset changes the implementation to make use of the ASTRewrite infrastructure, which automatically honors the const placement setting. Change-Id: Ib5ae9381b93ca8ab4d1ad3e16b1c3c8b1ec62d78 Signed-off-by: Felix Morgner --- .../CatchByConstReferenceQuickFix.java | 20 ++-- .../ui/quickfix/CatchByReferenceQuickFix.java | 112 +++++++++++------- .../CatchByReferenceQuickFixTest.java | 59 ++++++++- 3 files changed, 139 insertions(+), 52 deletions(-) diff --git a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByConstReferenceQuickFix.java b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByConstReferenceQuickFix.java index 635d9ceb1d6..e949ef550a0 100644 --- a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByConstReferenceQuickFix.java +++ b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByConstReferenceQuickFix.java @@ -11,22 +11,26 @@ *******************************************************************************/ package org.eclipse.cdt.codan.internal.checkers.ui.quickfix; +import java.util.Optional; + import org.eclipse.cdt.codan.internal.checkers.ui.Messages; -import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution; -import org.eclipse.core.resources.IMarker; -import org.eclipse.jface.text.IDocument; +import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier; +import org.eclipse.cdt.core.dom.ast.IASTNode.CopyStyle; +import org.eclipse.cdt.core.dom.ast.IASTSimpleDeclaration; /** * Quick fix for catch by value */ -public class CatchByConstReferenceQuickFix extends AbstractCodanCMarkerResolution { +public class CatchByConstReferenceQuickFix extends CatchByReferenceQuickFix { @Override public String getLabel() { return Messages.CatchByConstReferenceQuickFix_Message; } - @Override - public void apply(IMarker marker, IDocument document) { - CatchByReferenceQuickFix.applyCatchByReferenceQuickFix(marker, document, true); - } + protected Optional getNewDeclSpecifier(IASTSimpleDeclaration declaration) { + IASTDeclSpecifier declSpecifier = declaration.getDeclSpecifier(); + IASTDeclSpecifier replacement = declSpecifier.copy(CopyStyle.withLocations); + replacement.setConst(true); + return Optional.of(replacement); + } } diff --git a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFix.java b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFix.java index 300b98506af..4986b52ef73 100644 --- a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFix.java +++ b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFix.java @@ -10,65 +10,97 @@ *******************************************************************************/ package org.eclipse.cdt.codan.internal.checkers.ui.quickfix; +import java.util.Optional; + import org.eclipse.cdt.codan.internal.checkers.ui.CheckersUiActivator; import org.eclipse.cdt.codan.internal.checkers.ui.Messages; -import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution; +import org.eclipse.cdt.codan.ui.AbstractAstRewriteQuickFix; +import org.eclipse.cdt.core.dom.ast.ASTNodeFactoryFactory; +import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier; +import org.eclipse.cdt.core.dom.ast.IASTDeclarator; +import org.eclipse.cdt.core.dom.ast.IASTNode; +import org.eclipse.cdt.core.dom.ast.IASTNode.CopyStyle; +import org.eclipse.cdt.core.dom.ast.IASTSimpleDeclaration; +import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTReferenceOperator; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPNodeFactory; +import org.eclipse.cdt.core.dom.rewrite.ASTRewrite; +import org.eclipse.cdt.core.index.IIndex; +import org.eclipse.cdt.core.model.ITranslationUnit; import org.eclipse.core.resources.IMarker; -import org.eclipse.jface.text.BadLocationException; -import org.eclipse.jface.text.IDocument; +import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.NullProgressMonitor; /** * Quick fix for catch by value */ -public class CatchByReferenceQuickFix extends AbstractCodanCMarkerResolution { +public class CatchByReferenceQuickFix extends AbstractAstRewriteQuickFix { @Override public String getLabel() { return Messages.CatchByReferenceQuickFix_Message; } @Override - public void apply(IMarker marker, IDocument document) { - applyCatchByReferenceQuickFix(marker, document, false); - } + public void modifyAST(IIndex index, IMarker marker) { + IASTSimpleDeclaration declaration = getDeclaration(index, marker); + if (declaration == null) { + CheckersUiActivator.log("Could not find declaration"); //$NON-NLS-1$ + return; + } + + IASTTranslationUnit ast = declaration.getTranslationUnit(); + ASTRewrite rewrite = ASTRewrite.create(ast); + + getNewDeclSpecifier(declaration).ifPresent(ds -> rewrite.replace(declaration.getDeclSpecifier(), ds, null)); + rewrite.replace(declaration.getDeclarators()[0], getNewDeclarator(declaration), null); - static void applyCatchByReferenceQuickFix(IMarker marker, IDocument document, boolean addConst) { try { - int left = marker.getAttribute(IMarker.CHAR_START, -1); - int right = marker.getAttribute(IMarker.CHAR_END, -1); - String inStr = document.get(left, right - left); - document.replace(left, right - left, getCatchByReferenceString(inStr, addConst)); - } catch (BadLocationException e) { + rewrite.rewriteAST().perform(new NullProgressMonitor()); + } catch (CoreException e) { CheckersUiActivator.log(e); } } - + /** - * Returns a catch by reference string from a catch by value string + * Calculate the new IASTDeclSpecifier for the changed declaration + *

+ * Subclasses can override this method to provide custom behavior. + *

+ * + * @param declaration The original declaration + * @return A, possibly empty, {@link Optional} containing the new + * IASTDeclSpecifier */ - static private String getCatchByReferenceString(String inStr, boolean addConst) { - StringBuilder stringBuilder = new StringBuilder(inStr.length() + 10); - if (addConst) { - stringBuilder.append("const "); //$NON-NLS-1$ - } - - String typename; - int space = inStr.lastIndexOf(' '); - boolean hasDeclName = space != -1; - if (hasDeclName) { - typename = inStr.substring(0,space); - } else { - typename = inStr; - } - stringBuilder.append(typename); - - stringBuilder.append(" &"); //$NON-NLS-1$ - - if (hasDeclName) { - stringBuilder.append(" "); //$NON-NLS-1$ - String declname = inStr.substring(space+1); - stringBuilder.append(declname); - } - - return stringBuilder.toString(); + protected Optional getNewDeclSpecifier(IASTSimpleDeclaration declaration) { + return Optional.empty(); } + + private static IASTDeclarator getNewDeclarator(IASTSimpleDeclaration declaration) { + ICPPNodeFactory nodeFactory = ASTNodeFactoryFactory.getDefaultCPPNodeFactory(); + ICPPASTReferenceOperator reference = nodeFactory.newReferenceOperator(false); + IASTDeclarator declarator = declaration.getDeclarators()[0]; + IASTDeclarator replacement = declarator.copy(CopyStyle.withLocations); + replacement.addPointerOperator(reference); + return replacement; + } + + private IASTSimpleDeclaration getDeclaration(IIndex index, IMarker marker) { + try { + ITranslationUnit tu = getTranslationUnitViaEditor(marker); + IASTTranslationUnit ast = tu.getAST(index, ITranslationUnit.AST_SKIP_ALL_HEADERS); + int start = marker.getAttribute(IMarker.CHAR_START, -1); + int end = marker.getAttribute(IMarker.CHAR_END, -1); + if (start != -1 && end != -1) { + IASTNode node = ast.getNodeSelector(null).findNode(start, end - start); + while (node != null && !(node instanceof IASTSimpleDeclaration)) { + node = node.getParent(); + } + return (IASTSimpleDeclaration) node; + } + } catch (CoreException e) { + CheckersUiActivator.log(e); + } + return null; + } + } diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFixTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFixTest.java index d3dd2f85391..171f87c0246 100644 --- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFixTest.java +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFixTest.java @@ -12,6 +12,10 @@ package org.eclipse.cdt.codan.internal.checkers.ui.quickfix; import org.eclipse.cdt.codan.internal.checkers.CatchByReference; import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution; +import org.eclipse.cdt.core.CCorePlugin; +import org.eclipse.cdt.core.CCorePreferenceConstants; +import org.eclipse.core.resources.ProjectScope; +import org.eclipse.core.runtime.preferences.IEclipsePreferences; /** * @author Tomasz Wesolowski @@ -29,6 +33,16 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase { return true; } + private boolean setPlaceConstRight(boolean placeRight) { + IEclipsePreferences node = new ProjectScope(cproject.getProject()).getNode(CCorePlugin.PLUGIN_ID); + boolean before = node.getBoolean(CCorePreferenceConstants.PLACE_CONST_RIGHT_OF_TYPE, + CCorePreferenceConstants.DEFAULT_PLACE_CONST_RIGHT_OF_TYPE); + node.putBoolean(CCorePreferenceConstants.PLACE_CONST_RIGHT_OF_TYPE, placeRight); + CCorePreferenceConstants.getPreference(CCorePreferenceConstants.PLACE_CONST_RIGHT_OF_TYPE, cproject, + CCorePreferenceConstants.DEFAULT_PLACE_CONST_RIGHT_OF_TYPE); + return before; + } + @Override protected AbstractCodanCMarkerResolution createQuickFix() { return null; // quick fix to be chosen per test @@ -45,7 +59,7 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase { setQuickFix(new CatchByReferenceQuickFix()); loadcode(getAboveComment()); String result = runQuickFixOneFile(); - assertContainedIn("catch (C & exception)", result); //$NON-NLS-1$ + assertContainedIn("catch (C &exception)", result); //$NON-NLS-1$ } // struct C { @@ -59,7 +73,7 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase { setQuickFix(new CatchByReferenceQuickFix()); loadcode(getAboveComment()); String result = runQuickFixOneFile(); - assertContainedIn("catch (C &)", result); //$NON-NLS-1$ + assertContainedIn("catch (C&)", result); //$NON-NLS-1$ } // struct C { @@ -73,7 +87,25 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase { setQuickFix(new CatchByConstReferenceQuickFix()); loadcode(getAboveComment()); String result = runQuickFixOneFile(); - assertContainedIn("catch (const C & exception)", result); //$NON-NLS-1$ + assertContainedIn("catch (const C &exception)", result); //$NON-NLS-1$ + } + + // struct C { + // }; + // void foo() { + // try { + // } catch (C exception) { + // } + // } + public void testCatchByConstReferenceHonorsConstPlacementSettings_532120() throws Exception { + setQuickFix(new CatchByConstReferenceQuickFix()); + loadcode(getAboveComment()); + + boolean before = setPlaceConstRight(true); + String result = runQuickFixOneFile(); + setPlaceConstRight(before); + + assertContainedIn("catch (C const &exception)", result); //$NON-NLS-1$ } // struct C { @@ -87,6 +119,25 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase { setQuickFix(new CatchByConstReferenceQuickFix()); loadcode(getAboveComment()); String result = runQuickFixOneFile(); - assertContainedIn("catch (const C &)", result); //$NON-NLS-1$ + assertContainedIn("catch (const C&)", result); //$NON-NLS-1$ } + + // struct C { + // }; + // void foo() { + // try { + // } catch (C) { + // } + // } + public void testCatchByConstReferenceNoDeclNameHonorsConstPlacementSettings_532120() throws Exception { + setQuickFix(new CatchByConstReferenceQuickFix()); + loadcode(getAboveComment()); + + boolean before = setPlaceConstRight(true); + String result = runQuickFixOneFile(); + setPlaceConstRight(before); + + assertContainedIn("catch (C const &)", result); //$NON-NLS-1$ + } + }