From 87ae09ee72282f39509436d1e59529ac0356a7bb Mon Sep 17 00:00:00 2001 From: Sergey Prigogin Date: Tue, 12 Apr 2011 22:13:46 +0000 Subject: [PATCH] Bug 337486 - AbstractIndexAstChecker allows AST to outlive index read lock. --- .../internal/checkers/ReturnChecker.java | 5 +- .../cxx/model/AbstractIndexAstChecker.java | 244 +++++++++--------- .../codan/core/cxx/model/CxxModelsCache.java | 184 +++++++------ .../core/test/CodanFastCxxAstTestCase.java | 10 +- .../cdt/codan/core/model/AbstractChecker.java | 92 ++++--- .../cdt/codan/core/model/IChecker.java | 50 ++-- .../core/model/ICheckerInvocationContext.java | 46 +++- .../codan/core/model/ICodanDisposable.java | 32 +++ .../core/model/IRunnableInEditorChecker.java | 14 +- .../core/CheckerInvocationContext.java | 62 ++++- .../cdt/codan/internal/core/CodanBuilder.java | 65 ++--- 11 files changed, 477 insertions(+), 327 deletions(-) create mode 100644 codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICodanDisposable.java diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java index 5898623cc1e..f95c474367f 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java @@ -6,7 +6,7 @@ * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation *******************************************************************************/ package org.eclipse.cdt.codan.internal.checkers; @@ -14,7 +14,6 @@ import java.util.Iterator; import org.eclipse.cdt.codan.core.cxx.CxxAstUtils; import org.eclipse.cdt.codan.core.cxx.model.AbstractAstFunctionChecker; -import org.eclipse.cdt.codan.core.cxx.model.CxxModelsCache; import org.eclipse.cdt.codan.core.model.IProblem; import org.eclipse.cdt.codan.core.model.IProblemWorkingCopy; import org.eclipse.cdt.codan.core.model.cfg.ICfgData; @@ -156,7 +155,7 @@ public class ReturnChecker extends AbstractAstFunctionChecker { * @return */ protected boolean endsWithNoExitNode(IASTFunctionDefinition func) { - IControlFlowGraph graph = CxxModelsCache.getInstance().getControlFlowGraph(func); + IControlFlowGraph graph = getModelCache().getControlFlowGraph(func); Iterator exitNodeIterator = graph.getExitNodeIterator(); boolean noexitop = false; for (; exitNodeIterator.hasNext();) { diff --git a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java index b1874505c51..2a99930285b 100644 --- a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java +++ b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/AbstractIndexAstChecker.java @@ -6,13 +6,14 @@ * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.cxx.model; -import org.eclipse.cdt.codan.core.CodanCorePlugin; import org.eclipse.cdt.codan.core.cxx.Activator; import org.eclipse.cdt.codan.core.model.AbstractCheckerWithProblemPreferences; +import org.eclipse.cdt.codan.core.model.ICheckerInvocationContext; import org.eclipse.cdt.codan.core.model.IProblem; import org.eclipse.cdt.codan.core.model.IProblemLocation; import org.eclipse.cdt.codan.core.model.IProblemLocationFactory; @@ -23,114 +24,87 @@ import org.eclipse.cdt.core.dom.ast.IASTMacroExpansionLocation; import org.eclipse.cdt.core.dom.ast.IASTName; import org.eclipse.cdt.core.dom.ast.IASTNode; import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; -import org.eclipse.cdt.core.index.IIndex; -import org.eclipse.cdt.internal.core.resources.ResourceLookup; +import org.eclipse.cdt.core.model.CoreModel; +import org.eclipse.cdt.core.model.ICElement; +import org.eclipse.cdt.core.model.ITranslationUnit; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IProject; import org.eclipse.core.resources.IResource; import org.eclipse.core.runtime.CoreException; -import org.eclipse.core.runtime.IPath; -import org.eclipse.core.runtime.Path; +import org.eclipse.core.runtime.OperationCanceledException; /** - * Convenience implementation of checker that work on index based ast of a c/c++ + * Convenience implementation of checker that works on index-based AST of a C/C++ * program. * * Clients may extend this class. */ -public abstract class AbstractIndexAstChecker extends AbstractCheckerWithProblemPreferences implements ICAstChecker, - IRunnableInEditorChecker { - private IFile file; - private ICodanCommentMap commentmap; +public abstract class AbstractIndexAstChecker extends AbstractCheckerWithProblemPreferences + implements ICAstChecker, IRunnableInEditorChecker { + private CxxModelsCache modelCache; - protected IFile getFile() { - return file; - } - - protected IProject getProject() { - return file == null ? null : file.getProject(); - } - - void processFile(IFile file) throws CoreException, InterruptedException { - commentmap = null; - IASTTranslationUnit ast = CxxModelsCache.getInstance().getAst(file); - if (ast == null) - return; - // lock the index for read access - IIndex index = CxxModelsCache.getInstance().getIndex(file); - index.acquireReadLock(); - try { - // traverse the ast using the visitor pattern. - this.file = file; - processAst(ast); - } finally { - this.file = null; - index.releaseReadLock(); - } - } - - public synchronized boolean processResource(IResource resource) { + @Override + public synchronized boolean processResource(IResource resource) throws OperationCanceledException { if (!shouldProduceProblems(resource)) return false; - if (resource instanceof IFile) { - IFile file = (IFile) resource; + if (!(resource instanceof IFile)) + return true; + + processFile((IFile) resource); + return false; + } + + private void processFile(IFile file) throws OperationCanceledException { + ICheckerInvocationContext context = getContext(); + synchronized (context) { + modelCache = context.get(CxxModelsCache.class); + if (modelCache == null) { + ICElement celement = CoreModel.getDefault().create(file); + if (!(celement instanceof ITranslationUnit)) { + return; + } + modelCache = new CxxModelsCache((ITranslationUnit) celement); + context.add(modelCache); + } + } + + try { + IASTTranslationUnit ast = modelCache.getAST(); + if (ast != null) { + synchronized (ast) { + processAst(ast); + } + } + } catch (CoreException e) { + Activator.log(e); + } finally { + modelCache = null; + } + } + + /* + * (non-Javadoc) + * + * @see IRunnableInEditorChecker#processModel(Object, ICheckerInvocationContext) + */ + public synchronized void processModel(Object model, ICheckerInvocationContext context) { + if (model instanceof IASTTranslationUnit) { + setContext(context); + IASTTranslationUnit ast = (IASTTranslationUnit) model; + synchronized (context) { + modelCache = context.get(CxxModelsCache.class); + if (modelCache == null) { + modelCache = new CxxModelsCache(ast); + context.add(modelCache); + } + } try { - processFile(file); - } catch (CoreException e) { - CodanCorePlugin.log(e); - } catch (InterruptedException e) { - // ignore - } - return false; - } - return true; - } - - - public void reportProblem(String id, IASTNode astNode, Object... args) { - IProblemLocation loc = getProblemLocation(astNode); - if (loc!=null) reportProblem(id, loc, args); - } - public void reportProblem(IProblem problem, IASTNode astNode, Object... args) { - IProblemLocation loc = getProblemLocation(astNode); - if (loc!=null) reportProblem(problem, loc, args); - } - - @SuppressWarnings("restriction") - protected IProblemLocation getProblemLocation(IASTNode astNode) { - IASTFileLocation astLocation = astNode.getFileLocation(); - IPath location = new Path(astLocation.getFileName()); - IFile astFile = ResourceLookup.selectFileForLocation(location, getProject()); - if (astFile == null) { - astFile = file; - } - if (astFile == null) { - Activator.log("Cannot resolve location: " + location); //$NON-NLS-1$ - return null; - } - return getProblemLocation(astNode, astLocation, astFile); - } - - private IProblemLocation getProblemLocation(IASTNode astNode, IASTFileLocation astLocation, IFile astFile) { - int line = astLocation.getStartingLineNumber(); - IProblemLocationFactory locFactory = getRuntime().getProblemLocationFactory(); - if (hasMacroLocation(astNode) && astNode instanceof IASTName) { - IASTImageLocation imageLocation = ((IASTName) astNode).getImageLocation(); - if (imageLocation != null) { - int start = imageLocation.getNodeOffset(); - int end = start + imageLocation.getNodeLength(); - return locFactory.createProblemLocation(astFile, start, end, line); + processAst(ast); + } finally { + modelCache = null; + setContext(null); } } - if (line == astLocation.getEndingLineNumber()) { - return locFactory.createProblemLocation(astFile, astLocation.getNodeOffset(), - astLocation.getNodeOffset() + astLocation.getNodeLength(), line); - } - return locFactory.createProblemLocation(astFile, line); - } - - private boolean hasMacroLocation(IASTNode astNode) { - return astNode.getNodeLocations().length == 1 && astNode.getNodeLocations()[0] instanceof IASTMacroExpansionLocation; } @Override @@ -138,40 +112,60 @@ public abstract class AbstractIndexAstChecker extends AbstractCheckerWithProblem return true; } - /* - * (non-Javadoc) - * - * @see - * org.eclipse.cdt.codan.core.model.IRunnableInEditorChecker#processModel - * (java.lang.Object) - */ - @SuppressWarnings("restriction") - public synchronized void processModel(Object model) { - if (model instanceof IASTTranslationUnit) { - CxxModelsCache.getInstance().clearCash(); - IASTTranslationUnit ast = (IASTTranslationUnit) model; - IPath location = new Path(ast.getFilePath()); - IFile astFile = ResourceLookup.selectFileForLocation(location, getProject()); - file = astFile; - commentmap = null; - processAst(ast); + public void reportProblem(String id, IASTNode astNode, Object... args) { + IProblemLocation loc = getProblemLocation(astNode); + if (loc != null) + reportProblem(id, loc, args); + } + + public void reportProblem(IProblem problem, IASTNode astNode, Object... args) { + IProblemLocation loc = getProblemLocation(astNode); + if (loc != null) + reportProblem(problem, loc, args); + } + + protected IProblemLocation getProblemLocation(IASTNode astNode) { + IASTFileLocation astLocation = astNode.getFileLocation(); + return getProblemLocation(astNode, astLocation); + } + + private IProblemLocation getProblemLocation(IASTNode astNode, IASTFileLocation astLocation) { + int line = astLocation.getStartingLineNumber(); + IProblemLocationFactory locFactory = getRuntime().getProblemLocationFactory(); + if (hasMacroLocation(astNode) && astNode instanceof IASTName) { + IASTImageLocation imageLocation = ((IASTName) astNode).getImageLocation(); + if (imageLocation != null) { + int start = imageLocation.getNodeOffset(); + int end = start + imageLocation.getNodeLength(); + return locFactory.createProblemLocation(getFile(), start, end, line); + } } + if (line == astLocation.getEndingLineNumber()) { + return locFactory.createProblemLocation(getFile(), astLocation.getNodeOffset(), + astLocation.getNodeOffset() + astLocation.getNodeLength(), line); + } + return locFactory.createProblemLocation(getFile(), line); + } + + private boolean hasMacroLocation(IASTNode astNode) { + return astNode.getNodeLocations().length == 1 && + astNode.getNodeLocations()[0] instanceof IASTMacroExpansionLocation; + } + + protected IFile getFile() { + return modelCache.getFile(); + } + + protected IProject getProject() { + IFile file = getFile(); + return file == null ? null : file.getProject(); + } + + protected CxxModelsCache getModelCache() { + return modelCache; } protected ICodanCommentMap getCommentMap() { - if (commentmap == null) { - try { - CxxModelsCache cxxcache = CxxModelsCache.getInstance(); - synchronized (cxxcache) { - IASTTranslationUnit ast = cxxcache.getAst(getFile()); - commentmap = cxxcache.getCommentedNodeMap(ast); - return commentmap; - } - - } catch (Exception e) { - Activator.log(e); - } - } - return commentmap; + return modelCache.getCommentedNodeMap(); } } diff --git a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java index 6bf238782e6..63bf6095617 100644 --- a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java +++ b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/model/CxxModelsCache.java @@ -6,7 +6,8 @@ * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.cxx.model; @@ -15,32 +16,71 @@ import java.util.WeakHashMap; import org.eclipse.cdt.codan.core.cxx.Activator; import org.eclipse.cdt.codan.core.cxx.internal.model.CodanCommentMap; import org.eclipse.cdt.codan.core.cxx.internal.model.cfg.CxxControlFlowGraph; +import org.eclipse.cdt.codan.core.model.ICodanDisposable; import org.eclipse.cdt.codan.core.model.cfg.IControlFlowGraph; import org.eclipse.cdt.core.CCorePlugin; import org.eclipse.cdt.core.dom.ast.IASTFunctionDefinition; import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; import org.eclipse.cdt.core.index.IIndex; import org.eclipse.cdt.core.model.CoreModel; -import org.eclipse.cdt.core.model.ICElement; +import org.eclipse.cdt.core.model.ICProject; import org.eclipse.cdt.core.model.ITranslationUnit; import org.eclipse.cdt.internal.core.dom.rewrite.commenthandler.ASTCommenter; import org.eclipse.core.resources.IFile; +import org.eclipse.core.runtime.Assert; import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.OperationCanceledException; /** * Cache data models for resource so checkers can share it */ -public class CxxModelsCache { - private IFile file; - private IASTTranslationUnit ast; - private ITranslationUnit tu; - private IIndex index; - private WeakHashMap cfgmap = new WeakHashMap(0); - private ICodanCommentMap commentMap; - private static CxxModelsCache instance = new CxxModelsCache(); +public class CxxModelsCache implements ICodanDisposable { + private static final int PARSE_MODE = ITranslationUnit.AST_SKIP_ALL_HEADERS + | ITranslationUnit.AST_CONFIGURE_USING_SOURCE_CONTEXT + | ITranslationUnit.AST_SKIP_TRIVIAL_EXPRESSIONS_IN_AGGREGATE_INITIALIZERS + | ITranslationUnit.AST_PARSE_INACTIVE_CODE; - public static CxxModelsCache getInstance() { - return instance; + private final IFile file; + private final ITranslationUnit tu; + private IASTTranslationUnit ast; + private IIndex index; + private final WeakHashMap cfgmap; + private ICodanCommentMap commentMap; + private boolean disposed; + + CxxModelsCache(ITranslationUnit tu) { + this.tu = tu; + this.file = tu != null ? (IFile) tu.getResource() : null; + cfgmap = new WeakHashMap(0); + } + + CxxModelsCache(IASTTranslationUnit ast) { + this(ast.getOriginatingTranslationUnit()); + this.ast = ast; + } + + public IASTTranslationUnit getAST() throws OperationCanceledException, CoreException { + return getAST(tu); + } + + public IASTTranslationUnit getAST(ITranslationUnit tu) + throws OperationCanceledException, CoreException { + if (!this.tu.equals(tu)) { + throw new IllegalArgumentException(); + } + if (ast == null) { + getIndex(); + ast= tu.getAST(index, PARSE_MODE); + } + return ast; + } + + public ITranslationUnit getTranslationUnit() { + return tu; + } + + public IFile getFile() { + return file; } public synchronized IControlFlowGraph getControlFlowGraph(IASTFunctionDefinition func) { @@ -48,85 +88,67 @@ public class CxxModelsCache { if (cfg != null) return cfg; cfg = CxxControlFlowGraph.build(func); - if (cfgmap.size() > 20) { // if too many function better drop the cash XXX should be LRU + // TODO(Alena Laskavaia): Change to LRU. + if (cfgmap.size() > 20) { // if too many function better drop the cash cfgmap.clear(); } cfgmap.put(func, cfg); return cfg; } - public synchronized IASTTranslationUnit getAst(IFile file) throws CoreException, InterruptedException { - if (file.equals(this.file)) { - return ast; + public synchronized ICodanCommentMap getCommentedNodeMap() { + return getCommentedNodeMap(tu); + } + + public synchronized ICodanCommentMap getCommentedNodeMap(ITranslationUnit tu) { + if (!this.tu.equals(tu)) { + throw new IllegalArgumentException(); } - // create translation unit and access index - ICElement celement = CoreModel.getDefault().create(file); - if (!(celement instanceof ITranslationUnit)) - return null; // not a C/C++ file - clearCash(); - this.file = file; - //System.err.println("Making ast for "+file); - tu = (ITranslationUnit) celement; - index = CCorePlugin.getIndexManager().getIndex(tu.getCProject()); - // lock the index for read access - index.acquireReadLock(); - try { - // create index based ast - ast = tu.getAST(index, ITranslationUnit.AST_SKIP_INDEXED_HEADERS); - if (ast == null) - return null;// - return ast; - } finally { + if (commentMap == null) { + if (ast == null) { + throw new IllegalStateException("getCommentedNodeMap called before getAST"); //$NON-NLS-1$ + } + commentMap = new CodanCommentMap(ASTCommenter.getCommentedNodeMap(ast)); + } + return commentMap; + } + + /** + * Returns the index that can be safely used for reading until the cache is disposed. + * + * @return The index. + */ + public synchronized IIndex getIndex() throws CoreException, OperationCanceledException { + Assert.isTrue(!disposed, "CxxASTCache is already disposed."); //$NON-NLS-1$ + if (this.index == null) { + ICProject[] projects = CoreModel.getDefault().getCModel().getCProjects(); + IIndex index = CCorePlugin.getIndexManager().getIndex(projects); + try { + index.acquireReadLock(); + } catch (InterruptedException e) { + throw new OperationCanceledException(); + } + this.index = index; + } + return this.index; + } + + /** + * @see IDisposable#dispose() + * This method should not be called concurrently with any other method. + */ + public void dispose() { + Assert.isTrue(!disposed, "CxxASTCache.dispose() called more than once."); //$NON-NLS-1$ + disposed = true; + if (index != null) { index.releaseReadLock(); } } - - public synchronized ICodanCommentMap getCommentedNodeMap(IASTTranslationUnit ast) { - if (this.ast == ast) { - try { - index.acquireReadLock(); - try { - commentMap = new CodanCommentMap(ASTCommenter.getCommentedNodeMap(ast)); - } finally { - index.releaseReadLock(); - } - return commentMap; - } catch (InterruptedException e) { - return null; - } - } - throw new IllegalArgumentException("Not cached"); - } - - public ICodanCommentMap getCommentedNodeMap(IFile file) { - try { - IASTTranslationUnit ast = getAst(file); - return getCommentedNodeMap(ast); - } catch (InterruptedException e) { - return null; - } catch (CoreException e) { - Activator.log(e); - return null; - } - } - - /** - * Clear cash for current file - */ - public void clearCash() { - cfgmap.clear(); - ast = null; - tu = null; - index = null; - commentMap = null; - } - - public synchronized IIndex getIndex(IFile file) throws CoreException, InterruptedException { - if (file.equals(this.file)) { - return index; - } - getAst(file); // to init variables - return index; + @Override + protected void finalize() throws Throwable { + if (!disposed) + Activator.log("CxxASTCache was not disposed."); //$NON-NLS-1$ + super.finalize(); } } diff --git a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CodanFastCxxAstTestCase.java b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CodanFastCxxAstTestCase.java index 1bf928bd0a0..30b2d3e1e4f 100644 --- a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CodanFastCxxAstTestCase.java +++ b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/CodanFastCxxAstTestCase.java @@ -17,9 +17,11 @@ import junit.framework.TestCase; import org.eclipse.cdt.codan.core.CodanRuntime; import org.eclipse.cdt.codan.core.model.IChecker; +import org.eclipse.cdt.codan.core.model.ICheckerInvocationContext; import org.eclipse.cdt.codan.core.model.IProblemLocation; import org.eclipse.cdt.codan.core.model.IProblemReporter; import org.eclipse.cdt.codan.core.model.IRunnableInEditorChecker; +import org.eclipse.cdt.codan.internal.core.CheckerInvocationContext; import org.eclipse.cdt.core.dom.ast.IASTProblem; import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; import org.eclipse.cdt.core.dom.parser.ISourceCodeParser; @@ -71,7 +73,7 @@ public abstract class CodanFastCxxAstTestCase extends TestCase { /** * @return - * + * */ public IASTTranslationUnit parse(String code) { return parse(code, isCpp() ? ParserLanguage.CPP : ParserLanguage.C, true); @@ -116,7 +118,7 @@ public abstract class CodanFastCxxAstTestCase extends TestCase { /** * Override if any of code that test tried to parse has errors, otherwise * parse method would assert - * + * * @return */ protected boolean hasCodeErrors() { @@ -153,11 +155,13 @@ public abstract class CodanFastCxxAstTestCase extends TestCase { codanproblems.add(new ProblemInstance(problemId, loc, args)); } }); + ICheckerInvocationContext context = new CheckerInvocationContext(null); try { IChecker checker = getChecker(); - ((IRunnableInEditorChecker) checker).processModel(tu); + ((IRunnableInEditorChecker) checker).processModel(tu, context); } finally { CodanRuntime.getInstance().setProblemReporter(problemReporter); + context.dispose(); } } diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/AbstractChecker.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/AbstractChecker.java index 0fece960375..7382d865e46 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/AbstractChecker.java +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/AbstractChecker.java @@ -1,12 +1,13 @@ /******************************************************************************* - * Copyright (c) 2009, 2010 Alena Laskavaia + * Copyright (c) 2009, 2010 Alena Laskavaia * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.model; @@ -14,22 +15,21 @@ import java.util.ArrayList; import java.util.List; import org.eclipse.cdt.codan.core.CodanRuntime; -import org.eclipse.cdt.codan.internal.core.CheckerInvocationContext; import org.eclipse.cdt.codan.internal.core.CheckersRegistry; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IResource; +import org.eclipse.core.runtime.OperationCanceledException; /** * Convenience implementation of IChecker interface. Has a default * implementation for common methods. - * */ public abstract class AbstractChecker implements IChecker { - protected String name; /** * @since 2.0 */ - protected ICheckerInvocationContext context; + private ICheckerInvocationContext context; + private IProblemReporter problemReporter; /** * Default constructor @@ -47,7 +47,7 @@ public abstract class AbstractChecker implements IChecker { /** * Reports a simple problem for given file and line - * + * * @param id * - problem id * @param file @@ -66,7 +66,7 @@ public abstract class AbstractChecker implements IChecker { /** * Finds an instance of problem by given id, in user profile registered for * specific file - * + * * @param id * - problem id * @param file @@ -104,7 +104,7 @@ public abstract class AbstractChecker implements IChecker { /** * Reports a simple problem for given file and line, error message comes * from problem definition - * + * * @param id * - problem id * @param file @@ -121,16 +121,12 @@ public abstract class AbstractChecker implements IChecker { * @since 2.0 */ public IProblemReporter getProblemReporter() { - try { - return getContext().getProblemReporter(); - } catch (Exception e) { - return CodanRuntime.getInstance().getProblemReporter(); - } + return problemReporter; } /** * Convenience method to return codan runtime - * + * * @return */ protected CodanRuntime getRuntime() { @@ -139,7 +135,7 @@ public abstract class AbstractChecker implements IChecker { /** * Convenience method to create and return instance of IProblemLocation - * + * * @param file * - file where problem is found * @param line @@ -152,7 +148,7 @@ public abstract class AbstractChecker implements IChecker { /** * Convenience method to create and return instance of IProblemLocation - * + * * @param file * - file where problem is found * @param startChar @@ -176,7 +172,7 @@ public abstract class AbstractChecker implements IChecker { /** * report a problem - * + * * @param problemId - id of a problem * @param loc - problem location * @param args - extra problem arguments @@ -186,56 +182,72 @@ public abstract class AbstractChecker implements IChecker { } /** - * Get invocation context. - * + * Returns the invocation context. + * * @return checker invocation context - * + * * @since 2.0 */ - public ICheckerInvocationContext getContext() { + protected ICheckerInvocationContext getContext() { return context; } /** - * Set the invocation context. Usually called by codan builder. - * Object that calls this should also synchronize of checker object - * to prevent multi-thread access to a running context - * * @since 2.0 */ - public void setContext(ICheckerInvocationContext context) { + protected void setContext(ICheckerInvocationContext context) { this.context = context; } /** * @since 2.0 */ - public boolean before(IResource resource) { - IChecker checker = this; + public void before(IResource resource) { IProblemReporter problemReporter = CodanRuntime.getInstance().getProblemReporter(); - IProblemReporter sessionReporter = problemReporter; + this.problemReporter = problemReporter; if (problemReporter instanceof IProblemReporterSessionPersistent) { // create session problem reporter - sessionReporter = ((IProblemReporterSessionPersistent) problemReporter).createReporter(resource, checker); - ((IProblemReporterSessionPersistent) sessionReporter).start(); + this.problemReporter = ((IProblemReporterSessionPersistent) problemReporter).createReporter(resource, this); + ((IProblemReporterSessionPersistent) this.problemReporter).start(); } else if (problemReporter instanceof IProblemReporterPersistent) { // delete markers if checker can possibly run on this // resource this way if checker is not enabled markers would be // deleted too - ((IProblemReporterPersistent) problemReporter).deleteProblems(resource, checker); + ((IProblemReporterPersistent) problemReporter).deleteProblems(resource, this); } - ((AbstractChecker) checker).setContext(new CheckerInvocationContext(resource, sessionReporter)); - return true; } /** * @since 2.0 */ - public boolean after(IResource resource) { - if (getContext().getProblemReporter() instanceof IProblemReporterSessionPersistent) { - // delete general markers - ((IProblemReporterSessionPersistent) getContext().getProblemReporter()).done(); + public void after(IResource resource) { + if (problemReporter instanceof IProblemReporterSessionPersistent) { + // Delete general markers + ((IProblemReporterSessionPersistent) problemReporter).done(); + } + problemReporter = null; + } + + /** + * @param resource the resource to process. + * @return true if framework should traverse children of the resource and + * run this checkers on them again. + * @throws OperationCanceledException if the checker was interrupted. + * @since 2.0 + */ + public abstract boolean processResource(IResource resource) throws OperationCanceledException; + + /** + * @see IChecker#processResource(IResource, ICheckerInvocationContext) + * @since 2.0 + */ + public synchronized boolean processResource(IResource resource, ICheckerInvocationContext context) + throws OperationCanceledException { + this.setContext(context); + try { + return processResource(resource); + } finally { + this.setContext(null); } - return true; } } diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IChecker.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IChecker.java index af32d6eb6af..8ef378a970d 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IChecker.java +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IChecker.java @@ -1,27 +1,29 @@ /******************************************************************************* - * Copyright (c) 2009, 2010 Alena Laskavaia + * Copyright (c) 2009, 2010 Alena Laskavaia * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.model; import org.eclipse.core.resources.IResource; +import org.eclipse.core.runtime.OperationCanceledException; /** * Interface that checker must implement (through extending directly or * indirectly {@link AbstractChecker}. - * + * *

* EXPERIMENTAL. This class or interface has been added as part * of a work in progress. There is no guarantee that this API will work or that * it will remain the same. *

- * + * * @noextend This interface is not intended to be extended by clients. * @noimplement This interface is not intended to be implemented by clients. * Extend {@link AbstractChecker} class instead. @@ -29,25 +31,36 @@ import org.eclipse.core.resources.IResource; public interface IChecker { /** * Main method that checker should implement that actually detects errors - * - * @param resource - * - resource to run on + * + * @param resource the resource to run on. + * @param context container object for sharing data between different checkers + * operating on the resource. * @return true if framework should traverse children of the resource and - * run this checkers on them again - */ - boolean processResource(IResource resource); - - /** + * run this checkers on them again. + * @throws OperationCanceledException if the checker was interrupted. * @since 2.0 */ - boolean before(IResource resource); + boolean processResource(IResource resource, ICheckerInvocationContext context) + throws OperationCanceledException; /** + * Called before processing a resource. + * + * @param resource the resource that is about to be processed. * @since 2.0 */ - boolean after(IResource resource); + void before(IResource resource); /** + * Called before processing a resource. + * + * @param resource the resource that has been processed. + * @since 2.0 + */ + void after(IResource resource); + + /** + * @return the problem reporter. * @since 2.0 */ IProblemReporter getProblemReporter(); @@ -56,10 +69,9 @@ public interface IChecker { * Implement this method to trim down type of resource you are interested * in, usually it will be c/c++ files only. This method should be * independent from current user preferences. - * - * @param resource - * - resource to run on - * @return - true if checker should be run on this resource + * + * @param resource the resource to run on. + * @return true if checker should be run on this resource. */ boolean enabledInContext(IResource resource); @@ -70,7 +82,7 @@ public interface IChecker { * {@link IRunnableInEditorChecker}. * Checker should return false if check is non-trivial and takes a long * time. - * + * * @return true if need to be run in editor as user types, and false * otherwise */ diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICheckerInvocationContext.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICheckerInvocationContext.java index 057ee6abb88..adf04a95d8a 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICheckerInvocationContext.java +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICheckerInvocationContext.java @@ -1,34 +1,60 @@ /******************************************************************************* - * Copyright (c) 2009,2010 QNX Software Systems + * Copyright (c) 2009, 2011 QNX Software Systems * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * QNX Software Systems (Alena Laskavaia) - initial API and implementation + * QNX Software Systems (Alena Laskavaia) - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.model; import org.eclipse.core.resources.IResource; /** - * Since there is only one instance of checker available this object keeps - * track of invocation context - which would usually contain resource and some - * other object that checker require + * Context object that can be used to store data shared between different + * checkers operating on the same resource. The context and all objects stored + * in it are disposed of at the end of processing of a resource. May store + * objects of arbitrary types but only a single instance per type. + *

+ * Implementations of this interface are guaranteed to be thread-safe. *

* EXPERIMENTAL. This class or interface has been added as part * of a work in progress. There is no guarantee that this API will work or that * it will remain the same. - *

- * + * * @noextend This interface is not intended to be extended by clients. * @noimplement This interface is not intended to be implemented by clients. - * + * * @since 2.0 */ -public interface ICheckerInvocationContext { +public interface ICheckerInvocationContext extends ICodanDisposable { + /** + * @return the resource this context is associated with. + */ IResource getResource(); - IProblemReporter getProblemReporter(); + /** + * Returns the object of the given type. Lookup by an interface or a superclass + * is also possible, but in case when there are multiple objects implementing + * the interface, an arbitrary one will be returned. + * + * @param the type of the object. + * @param objectClass the class of the object to retrieve. + * @return the object of the given type, or null if not present in the context. + */ + public T get(Class objectClass); + + /** + * Adds an object to the context. The context accepts only a single + * instance of each class. + * + * @param the type of the object. + * @param object the object to add to the context. + * @throws IllegalArgumentException if an attempt is made to add second instance + * of the same class. + */ + public void add(T object); } diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICodanDisposable.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICodanDisposable.java new file mode 100644 index 00000000000..a175c058090 --- /dev/null +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/ICodanDisposable.java @@ -0,0 +1,32 @@ +/******************************************************************************* + * Copyright (c) 2011 Google, Inc and others. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Sergey Prigogin (Google) - initial API and implementation + *******************************************************************************/ +package org.eclipse.cdt.codan.core.model; + +/** + *

+ * An interface for objects that own resources that have to be explicitly + * released. A disposable object is guaranteed to receive a {@link #dispose()} call + * when it is not longer needed. At this point, the object must release all resources + * and detach all listeners. A disposable object can only be disposed once; it cannot + * be reused. + *

+ *

+ * This interface can be extended or implemented by clients. + *

+ * @since 2.0 + */ +public interface ICodanDisposable { + /** + * Disposes of the cache. This method has to be called exactly once during + * the life cycle of the cache. + */ + public void dispose(); +} diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IRunnableInEditorChecker.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IRunnableInEditorChecker.java index 694aaf42b09..f89c226e879 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IRunnableInEditorChecker.java +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/core/model/IRunnableInEditorChecker.java @@ -1,12 +1,13 @@ /******************************************************************************* - * Copyright (c) 2009, 2010 Alena Laskavaia + * Copyright (c) 2009, 2010 Alena Laskavaia * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.core.model; @@ -18,12 +19,15 @@ package org.eclipse.cdt.codan.core.model; * of a work in progress. There is no guarantee that this API will work or that * it will remain the same. *

- * + * * @noextend This interface is not intended to be extended by clients. */ public interface IRunnableInEditorChecker { /** - * @param model + * @param model the model to check. + * @param context container object for sharing data between different checkers + * operating on the model. + * @since 2.0 */ - void processModel(Object model); + void processModel(Object model, ICheckerInvocationContext context); } diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CheckerInvocationContext.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CheckerInvocationContext.java index 2a48927ae31..c0d8c6b73ed 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CheckerInvocationContext.java +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CheckerInvocationContext.java @@ -1,40 +1,78 @@ /******************************************************************************* - * Copyright (c) 2009,2010 QNX Software Systems + * Copyright (c) 2009, 2011 QNX Software Systems * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * QNX Software Systems (Alena Laskavaia) - initial API and implementation + * QNX Software Systems (Alena Laskavaia) - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.internal.core; +import java.util.HashMap; +import java.util.Map; + import org.eclipse.cdt.codan.core.model.ICheckerInvocationContext; -import org.eclipse.cdt.codan.core.model.IProblemReporter; +import org.eclipse.cdt.codan.core.model.ICodanDisposable; import org.eclipse.core.resources.IResource; /** - * Implementation of ICheckerInvocationContext + * Implementation of ICheckerInvocationContext. + * This class is thread-safe. */ public class CheckerInvocationContext implements ICheckerInvocationContext { - private IResource resource; - private IProblemReporter sessionReporter; + private final IResource resource; + private final Map, Object> objectStorage; /** - * @param resource - * @param sessionReporter + * @param resource the resource this context is associated with. */ - public CheckerInvocationContext(IResource resource, IProblemReporter sessionReporter) { + public CheckerInvocationContext(IResource resource) { this.resource = resource; - this.sessionReporter = sessionReporter; + objectStorage = new HashMap, Object>(); } public IResource getResource() { return resource; } - public IProblemReporter getProblemReporter() { - return sessionReporter; + @SuppressWarnings("unchecked") + public synchronized T get(Class objectClass) { + T object = (T) objectStorage.get(objectClass); + if (object != null) + return object; + for (Map.Entry, Object> entry : objectStorage.entrySet()) { + if (objectClass.isAssignableFrom(entry.getKey())) + return (T) entry.getValue(); + } + return null; + } + + /* + * (non-Javadoc) + * @see ICheckerInvocationContext#add(Object) + */ + public synchronized void add(T object) { + Object old = objectStorage.put(object.getClass(), object); + if (old != null && object != old) { + objectStorage.put(old.getClass(), old); // Restore old value. + throw new IllegalArgumentException(); + } + } + + /* + * (non-Javadoc) + * @see IDisposableCache#dispose() + */ + public void dispose() { + for (Map.Entry, Object> entry : objectStorage.entrySet()) { + Object obj = entry.getValue(); + if (obj instanceof ICodanDisposable) { + ((ICodanDisposable) obj).dispose(); + } + } + objectStorage.clear(); } } diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CodanBuilder.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CodanBuilder.java index 4de58b2b010..1aa940b9c19 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CodanBuilder.java +++ b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/CodanBuilder.java @@ -6,7 +6,8 @@ * http://www.eclipse.org/legal/epl-v10.html * * Contributors: - * Alena Laskavaia - initial API and implementation + * Alena Laskavaia - initial API and implementation + * Sergey Prigogin (Google) *******************************************************************************/ package org.eclipse.cdt.codan.internal.core; @@ -17,6 +18,7 @@ import org.eclipse.cdt.codan.core.Messages; import org.eclipse.cdt.codan.core.model.CheckerLaunchMode; import org.eclipse.cdt.codan.core.model.Checkers; import org.eclipse.cdt.codan.core.model.IChecker; +import org.eclipse.cdt.codan.core.model.ICheckerInvocationContext; import org.eclipse.cdt.codan.core.model.ICodanBuilder; import org.eclipse.cdt.codan.core.model.IRunnableInEditorChecker; import org.eclipse.core.resources.IContainer; @@ -27,6 +29,7 @@ import org.eclipse.core.resources.IResourceDeltaVisitor; import org.eclipse.core.resources.IncrementalProjectBuilder; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.OperationCanceledException; import org.eclipse.core.runtime.SubProgressMonitor; /** @@ -34,7 +37,7 @@ import org.eclipse.core.runtime.SubProgressMonitor; */ public class CodanBuilder extends IncrementalProjectBuilder implements ICodanBuilder { /** - * codan builder id + * Codan builder id */ public static final String BUILDER_ID = "org.eclipse.cdt.codan.core.codanBuilder"; //$NON-NLS-1$ @@ -126,39 +129,43 @@ public class CodanBuilder extends IncrementalProjectBuilder implements ICodanBui // System.err.println("processing " + resource); monitor.beginTask(Messages.CodanBuilder_Code_Analysis_On + resource, checkers + memsize * tick); try { - for (IChecker checker : chegistry) { - try { - if (monitor.isCanceled()) - return; - if (doesCheckerSupportLaunchMode(checker, checkerLaunchMode) - && checker.enabledInContext(resource) - && chegistry.isCheckerEnabledForLaunchMode(checker, resource, checkerLaunchMode)) { - synchronized (checker) { - try { - checker.before(resource); - if (chegistry.isCheckerEnabled(checker, resource)) { - //long time = System.currentTimeMillis(); - if (checkerLaunchMode == CheckerLaunchMode.RUN_AS_YOU_TYPE) { - ((IRunnableInEditorChecker) checker).processModel(model); - } else { - checker.processResource(resource); + ICheckerInvocationContext context = new CheckerInvocationContext(resource); + try { + for (IChecker checker : chegistry) { + try { + if (monitor.isCanceled()) + return; + if (doesCheckerSupportLaunchMode(checker, checkerLaunchMode) + && checker.enabledInContext(resource) + && chegistry.isCheckerEnabledForLaunchMode(checker, resource, checkerLaunchMode)) { + synchronized (checker) { + try { + checker.before(resource); + if (chegistry.isCheckerEnabled(checker, resource)) { + //long time = System.currentTimeMillis(); + if (checkerLaunchMode == CheckerLaunchMode.RUN_AS_YOU_TYPE) { + ((IRunnableInEditorChecker) checker).processModel(model, context); + } else { + checker.processResource(resource, context); + } + // System.err.println("Checker " + // + checker.getClass() + " worked " + // + (System.currentTimeMillis() - time)); } - // System.err - // .println("Checker " - // + checker.getClass() - // + " worked " - // + (System - // .currentTimeMillis() - time)); + } finally { + checker.after(resource); } - } finally { - checker.after(resource); } } + monitor.worked(1); + } catch (OperationCanceledException e) { + return; + } catch (Throwable e) { + CodanCorePlugin.log(e); } - monitor.worked(1); - } catch (Throwable e) { - CodanCorePlugin.log(e); } + } finally { + context.dispose(); } if (resource instanceof IContainer && (checkerLaunchMode == CheckerLaunchMode.RUN_ON_FULL_BUILD || checkerLaunchMode == CheckerLaunchMode.RUN_ON_DEMAND)) {