Bugzilla – Full Text Bug Listing
| Summary: | test failures and indexing exception in LR Parser | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | John Liu <john_ws_liu> |
| Component: | cdt-parser | Assignee: | Vivian Kong <vivkong> |
| Status: | RESOLVED FIXED | QA Contact: | Mike Kucera <mikekucera> |
| Severity: | normal | ||
| Priority: | P3 | CC: | mschorn.eclipse, recoskie, vivkong |
| Version: | 7.0 | ||
| Target Milestone: | 7.0.1 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
John Liu
Created attachment 175595 [details]
cdt_7_0 patch applying to cdt.core and cdt.core.lrparser
two modifications for this patch
1, Add a exception catching block in writeToIndex function of AbstractIndexerTask, this is because we see exception thrown from here by standalone indexer.
2,Update consumeTemplateArgumentExpression function in CPPBuildASTParserAction to avoid unsupport copy action.
Assigning to myself for now The patch needs to be reworked: * Using an ASTName twice in the tree does not work, it will result in a wrong child - parent relationship. * The CoreException must not be caught in AbstractIndexerTask.writeToIndex(..), these exceptions are handled in AbstractIndexerTask.parseFile(...). (In reply to comment #3) > The patch needs to be reworked: > * Using an ASTName twice in the tree does not work, it will result in a wrong > child - parent relationship. > * The CoreException must not be caught in AbstractIndexerTask.writeToIndex(..), > these exceptions are handled in AbstractIndexerTask.parseFile(...). Thanks Markus, for your comments. The original problem of throwing UnsupportedOperationException is because CPPASTAmbiguousTemplateArgument doesn't support copy function. So if we have a template which has another template as its argument, then we get this exception. In this case, using original ASTName in the ambiguityNode is a closest solution before we have copy function implemented in CPPASTAmbiguousTemplateArgument. If ASTName is copied well, we won't use it twice. In CDT 50, the ASTName is copied shallowly, which cause template lost it arguments in a template of template case. I guess there must be a difficulty to implement the copy function of CPPASTAmbiguousTemplateArgument, I don't feel comfortable to add its implementation since I am not familiar with AST implementation. Do you know a reason why we don't implement it and is it possible to add it soon? Also would you accept this workaround fix for now? it passed all of junit tests. For CoreException, I agree putting a catch in writeToIndex() is not a right fix. I will rework this fix. I see parseFile() passes exception to swallowError() to handle, but in one of out large project, we still see the exception is thrown from writeToIndex().(please see exception stack at the end) The exception's status must be CCorePlugin.STATUS_PDOM_TOO_LARGE since it is thorwn again by swallowError(). I try to find out what cause an exception in CCorePlugin.STATUS_PDOM_TOO_LARGE status, I then find out org.eclipse.cdt.internal.core.pdom.db.datbase has a MAX_DB_SIZE. My questions here are: 1) can we enlarge the MAX_DB_SIZE? 2) if not, can we handle STATUS_PDOM_TOO_LARGE exception in a different way to avoid throwing it again? org.eclipse.core.runtime.CoreException: at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:271) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.adaptOrAddBinding(PDOMCPPLinkage.java:695) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.adaptOrAddParent(PDOMCPPLinkage.java:687) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:256) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:220) at org.eclipse.cdt.internal.core.pdom.dom.PDOMFile.createPDOMName(PDOMFile.java:427) at org.eclipse.cdt.internal.core.pdom.dom.PDOMFile.addNames(PDOMFile.java:394) at org.eclipse.cdt.internal.core.pdom.WritablePDOM.addFileContent(WritablePDOM.java:125) at org.eclipse.cdt.internal.core.index.WritableCIndex.setFileContent(WritableCIndex.java:86) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.storeFileInIndex(PDOMWriter.java:504) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.storeSymbolsInIndex(PDOMWriter.java:210) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.addSymbols(PDOMWriter.java:165) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.writeToIndex(AbstractIndexerTask.java:798) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.parseFile(AbstractIndexerTask.java:756) (In reply to comment #4) > (In reply to comment #3) > The original problem of throwing UnsupportedOperationException is because > CPPASTAmbiguousTemplateArgument doesn't support copy function. So if we have a > template which has another template as its argument, then we get this > exception. In this case, using original ASTName in the ambiguityNode is a > closest solution before we have copy function implemented in > CPPASTAmbiguousTemplateArgument. If ASTName is copied well, we won't use it > twice. In CDT 50, the ASTName is copied shallowly, which cause template lost it > arguments in a template of template case. > I guess there must be a difficulty to implement the copy function of > CPPASTAmbiguousTemplateArgument, I don't feel comfortable to add its > implementation since I am not familiar with AST implementation. Do you know a > reason why we don't implement it and is it possible to add it soon? Also would > you accept this workaround fix for now? it passed all of junit tests. The copy() method was created for clients like the refactoring. At the time when clients get to work with the AST the ambiguity nodes are resolved. Therefore these nodes do not have an implementation for copy(). If the LR-parsers really need ot use copy() (the standard parsers don't) you need to add the missing implementations. I am not maintaining the LR-parsers, so it's up to you on how you want to fix them. I don't review or apply changes to the LR-parser code. Nevertheless the proposed fix is a hack. > For CoreException, I agree putting a catch in writeToIndex() is not a right > fix. I will rework this fix. > I see parseFile() passes exception to swallowError() to handle, but in one of > out large project, we still see the exception is thrown from > writeToIndex().(please see exception stack at the end) The exception's status > must be CCorePlugin.STATUS_PDOM_TOO_LARGE since it is thorwn again by > swallowError().... The exception is also rethrown when the number of exceptions for a translation unit exceeds a certain limit. This is done to make sure the parser does not get stuck in a situation where something goes obviously wrong. You need to look at the root cause of the exceptions. The copy() method is used in the actions only as a convenience. At one point I was writing some code that manually created an AST fragment and I realized I could make the code a lot shorter by copying some chunks of existing AST. But the copy() method is certainly not required by the actions, like I said its only used for convenience. If you ran into a bug as the result of calling copy() then perhaps all uses of that method should be removed from the parser. You should take a deeper look into why copy() is being called. Its probably an attempt to resolve some ambiguity by manually creating an ambiguity node. If this is the case then there may be better ways to resolve it. (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > The original problem of throwing UnsupportedOperationException is because > > CPPASTAmbiguousTemplateArgument doesn't support copy function. So if we have a > > template which has another template as its argument, then we get this > > exception. In this case, using original ASTName in the ambiguityNode is a > > closest solution before we have copy function implemented in > > CPPASTAmbiguousTemplateArgument. If ASTName is copied well, we won't use it > > twice. In CDT 50, the ASTName is copied shallowly, which cause template lost it > > arguments in a template of template case. > > I guess there must be a difficulty to implement the copy function of > > CPPASTAmbiguousTemplateArgument, I don't feel comfortable to add its > > implementation since I am not familiar with AST implementation. Do you know a > > reason why we don't implement it and is it possible to add it soon? Also would > > you accept this workaround fix for now? it passed all of junit tests. > The copy() method was created for clients like the refactoring. At the time > when clients get to work with the AST the ambiguity nodes are resolved. > Therefore these nodes do not have an implementation for copy(). > If the LR-parsers really need ot use copy() (the standard parsers don't) you > need to add the missing implementations. > I am not maintaining the LR-parsers, so it's up to you on how you want to fix > them. I don't review or apply changes to the LR-parser code. Nevertheless the > proposed fix is a hack. > > For CoreException, I agree putting a catch in writeToIndex() is not a right > > fix. I will rework this fix. > > I see parseFile() passes exception to swallowError() to handle, but in one of > > out large project, we still see the exception is thrown from > > writeToIndex().(please see exception stack at the end) The exception's status > > must be CCorePlugin.STATUS_PDOM_TOO_LARGE since it is thorwn again by > > swallowError().... > The exception is also rethrown when the number of exceptions for a translation > unit exceeds a certain limit. This is done to make sure the parser does not get > stuck in a situation where something goes obviously wrong. You need to look at > the root cause of the exceptions. OK, I will add an implementation of copy() to CPPASTAmbiguousTemplateArgument. For CoreException, I believe it is a CCorePlugin.STATUS_PDOM_TOO_LARGE exception, from our exception stack, it is thrown from the line of writeToIndex() (AbstractIndexerTask.java:798). This is because swallowError() throws the original exception for CCorePlugin.STATUS_PDOM_TOO_LARGE exception. If it is caused by MessageKind.tooManyIndexProblems problem, it would be thrown from swallowError() function, line 896, since swallowError() creates a new CoreException over there for MessageKind.tooManyIndexProblems problem. Please correct me if my analysis is wrong. Also in my test with the hacking catch in writeToIndex(), I see indexing can be finished fine, although there are some problems reported. So should we let indexing gives a strong warning rather than stopping it when it meets CCorePlugin.STATUS_PDOM_TOO_LARGE exception? This should be a rare case only when the project is huge, but we would prefer to having a result even if it may have problems. Created attachment 175882 [details]
a patch to be applied to org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTAmbiguousTemplateArgument
The patch implements copy function in CPPASTAmbiguousTemplateArgument class.
(In reply to comment #8) > Created an attachment (id=175882) [details] > a patch to be applied to > org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTAmbiguousTemplateArgument > The patch implements copy function in CPPASTAmbiguousTemplateArgument class. Please let me know if anyone has any concerns regarding the patch. It fixs UnsupportedOperationException in LR parser test. (In reply to comment #9) > Please let me know if anyone has any concerns regarding the patch. It fixs > UnsupportedOperationException in LR parser test. - There is no need to create a list and then convert the list into an array after. You know the size of the array ahead of time so just populate an array in the first place. - Should each node be checked for null? I think all the copy() methods allow children to be null. With this patch if the node has a null child you'll get an NPE. - Needs a JUnit. - For the sake of completeness all the ambiguity nodes should have copy() implemented. Its kind of strange that only one out of all the ambiguity nodes would support copy(). Created attachment 175889 [details]
a updated patch to be applied to org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTAmbiguousTemplateArgument
Thanks, Mike.
Update it for your first two points. Will look at other ambiguous node and junit test case for them later. Probably need another bugzilla. This is to address the found LR parser test failures.
> ... > For CoreException, I believe it is a CCorePlugin.STATUS_PDOM_TOO_LARGE > exception, from our exception stack. > ... > Also in my test with the hacking catch in writeToIndex(), I see indexing can > be finished fine, although there are some problems reported. So should we > let indexing gives a strong warning rather than stopping it when it meets > CCorePlugin.STATUS_PDOM_TOO_LARGE exception? This should be a rare case only > when the project is huge, but we would prefer to having a result even if > it may have problems. When the index grows beyond 2GB, indexing must be stopped. For more information, see bug 279620. (In reply to comment #11) > Created an attachment (id=175889) [details] > a updated patch to be applied to > org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTAmbiguousTemplateArgument > Thanks, Mike. > Update it for your first two points. Will look at other ambiguous node and > junit test case for them later. Probably need another bugzilla. This is to > address the found LR parser test failures. It is not a problem if the test-case fails for a while, we can wait until you have a complete fix for the actual problem. I prefer the failing test-case over an incomplete fix that is likely to become the permanent solution. Created attachment 176099 [details]
2 fixes applied to cdt.core(see comments for details)
Find another two problems related to the exceptions we found.
1) CCorePlugin.STATUS_PDOM_TOO_LARGE has same value with IStatus.ERROR, they are both a status code flag. In StandaloneIndexerTask, the function
protected IStatus createStatus(String msg, Throwable e) {
return new Status(IStatus.ERROR, "org.eclipse.cdt.core", IStatus.ERROR, msg, e);
}
create staus using IStatus.ERROR as its code. So the exception status could be consider as a CCorePlugin.STATUS_PDOM_TOO_LARGE status.
Proposed the fis in the patch to chnage CCorePlugin.STATUS_PDOM_TOO_LARGE value to 104.
2) The function of findOverloadedOperator in class CPPSemantics should check if the op2 is null since CPPASTBinaryExpression could return null by its getOperand2 function.
Should anyone have any concern regarding the propsed fix, please let me know. Markus, please let me know your comments if you have any.
Thanks.
(In reply to comment #12) > > ... > > For CoreException, I believe it is a CCorePlugin.STATUS_PDOM_TOO_LARGE > > exception, from our exception stack. > > ... > > Also in my test with the hacking catch in writeToIndex(), I see indexing can > > be finished fine, although there are some problems reported. So should we > > let indexing gives a strong warning rather than stopping it when it meets > > CCorePlugin.STATUS_PDOM_TOO_LARGE exception? This should be a rare case only > > when the project is huge, but we would prefer to having a result even if > > it may have problems. > When the index grows beyond 2GB, indexing must be stopped. For more > information, see bug 279620 It is not a CCorePlugin.STATUS_PDOM_TOO_LARGE exception, it could be a false alarm due to the status code collision between CCorePlugin.STATUS_PDOM_TOO_LARGE AND IStatus.ERROR. See my comments in my proposed fixes attachment. However I still get some coreExceptions in standard error after applying my fix, the exceptions can not be MessageKind.tooManyIndexProblems exceptions because they have a nested DOMException exception. See the following error stack. It is so weird that some coreExceptions (other than CCorePlugin.STATUS_PDOM_TOO_LARGE and MessageKind.tooManyIndexProblems exceptions) can still got thrown from parseFile(). Markus, do you have any observations here? org.eclipse.core.runtime.CoreException: at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:271) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:220) at org.eclipse.cdt.internal.core.pdom.dom.PDOMFile.createPDOMName(PDOMFile.java:427) at org.eclipse.cdt.internal.core.pdom.dom.PDOMFile.addNames(PDOMFile.java:394) at org.eclipse.cdt.internal.core.pdom.WritablePDOM.addFileContent(WritablePDOM.java:125) at org.eclipse.cdt.internal.core.index.WritableCIndex.setFileContent(WritableCIndex.java:86) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.storeFileInIndex(PDOMWriter.java:504) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.storeSymbolsInIndex(PDOMWriter.java:210) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.addSymbols(PDOMWriter.java:165) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.writeToIndex(AbstractIndexerTask.java:803) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.parseFile(AbstractIndexerTask.java:759) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.parseLinkage(AbstractIndexerTask.java:639) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.runTask(AbstractIndexerTask.java:346) at org.eclipse.cdt.internal.core.indexer.StandaloneIndexerTask.run(StandaloneIndexerTask.java:131) at org.eclipse.cdt.internal.core.indexer.StandaloneIndexer.rebuild(StandaloneIndexer.java:409) at org.eclipse.ptp.internal.rdt.core.miners.IndexerThread$1.runIndexer(IndexerThread.java:48) at org.eclipse.ptp.internal.rdt.core.miners.IndexerThread.run(IndexerThread.java:71) Caused by: org.eclipse.cdt.core.dom.ast.DOMException at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPParameter$CPPParameterProblem.getType(CPPParameter.java:50) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPFunction.getRequiredArgumentCount(CPPFunction.java:599) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPFunction.getRequiredArgumentCount(CPPFunction.java:589) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPFunction.<init>(PDOMCPPFunction.java:95) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.createBinding(PDOMCPPLinkage.java:363) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:266) ... 16 more (In reply to comment #12) > > ... > > For CoreException, I believe it is a CCorePlugin.STATUS_PDOM_TOO_LARGE > > exception, from our exception stack. > > ... > > Also in my test with the hacking catch in writeToIndex(), I see indexing can > > be finished fine, although there are some problems reported. So should we > > let indexing gives a strong warning rather than stopping it when it meets > > CCorePlugin.STATUS_PDOM_TOO_LARGE exception? This should be a rare case only > > when the project is huge, but we would prefer to having a result even if > > it may have problems. > When the index grows beyond 2GB, indexing must be stopped. For more > information, see bug 279620 It is not a CCorePlugin.STATUS_PDOM_TOO_LARGE exception, it could be a false alarm due to the status code collision between CCorePlugin.STATUS_PDOM_TOO_LARGE AND IStatus.ERROR. See my comments in my proposed fixes attachment. However I still get some coreExceptions in standard error after applying my fix, the exceptions can not be MessageKind.tooManyIndexProblems exceptions because they have a nested DOMException exception. See the following error stack. It is so weird that some coreExceptions (other than CCorePlugin.STATUS_PDOM_TOO_LARGE and MessageKind.tooManyIndexProblems exceptions) can still got thrown from parseFile(). Markus, do you have any observations here? org.eclipse.core.runtime.CoreException: at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:271) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:220) at org.eclipse.cdt.internal.core.pdom.dom.PDOMFile.createPDOMName(PDOMFile.java:427) at org.eclipse.cdt.internal.core.pdom.dom.PDOMFile.addNames(PDOMFile.java:394) at org.eclipse.cdt.internal.core.pdom.WritablePDOM.addFileContent(WritablePDOM.java:125) at org.eclipse.cdt.internal.core.index.WritableCIndex.setFileContent(WritableCIndex.java:86) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.storeFileInIndex(PDOMWriter.java:504) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.storeSymbolsInIndex(PDOMWriter.java:210) at org.eclipse.cdt.internal.core.pdom.PDOMWriter.addSymbols(PDOMWriter.java:165) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.writeToIndex(AbstractIndexerTask.java:803) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.parseFile(AbstractIndexerTask.java:759) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.parseLinkage(AbstractIndexerTask.java:639) at org.eclipse.cdt.internal.core.pdom.AbstractIndexerTask.runTask(AbstractIndexerTask.java:346) at org.eclipse.cdt.internal.core.indexer.StandaloneIndexerTask.run(StandaloneIndexerTask.java:131) at org.eclipse.cdt.internal.core.indexer.StandaloneIndexer.rebuild(StandaloneIndexer.java:409) at org.eclipse.ptp.internal.rdt.core.miners.IndexerThread$1.runIndexer(IndexerThread.java:48) at org.eclipse.ptp.internal.rdt.core.miners.IndexerThread.run(IndexerThread.java:71) Caused by: org.eclipse.cdt.core.dom.ast.DOMException at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPParameter$CPPParameterProblem.getType(CPPParameter.java:50) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPFunction.getRequiredArgumentCount(CPPFunction.java:599) at org.eclipse.cdt.internal.core.dom.parser.cpp.CPPFunction.getRequiredArgumentCount(CPPFunction.java:589) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPFunction.<init>(PDOMCPPFunction.java:95) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.createBinding(PDOMCPPLinkage.java:363) at org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPLinkage.addBinding(PDOMCPPLinkage.java:266) ... 16 more (In reply to comment #14) > Created an attachment (id=176099) [details] > 2 fixes applied to cdt.core(see comments for details) > Find another two problems related to the exceptions we found. > 1) CCorePlugin.STATUS_PDOM_TOO_LARGE has same value with IStatus.ERROR, they > are both a status code flag. > In StandaloneIndexerTask, the function > protected IStatus createStatus(String msg, Throwable e) { > return new Status(IStatus.ERROR, "org.eclipse.cdt.core", IStatus.ERROR, > msg, e); > } > create staus using IStatus.ERROR as its code. So the exception status could be > consider as a CCorePlugin.STATUS_PDOM_TOO_LARGE status. > Proposed the fis in the patch to chnage CCorePlugin.STATUS_PDOM_TOO_LARGE value > to 104. > 2) The function of findOverloadedOperator in class CPPSemantics should check if > the op2 is null since CPPASTBinaryExpression could return null by its > getOperand2 function. > Should anyone have any concern regarding the propsed fix, please let me know. > Markus, please let me know your comments if you have any. > Thanks. IStatus.ERROR is a severity, CCorePlugin.STATUS_PDOM_TOO_LARGE is a status code, they can have the same value. The method createStatus needs to be changed. I know that the specific status code was an unlucky choice (see bug 293021), however it is public API and therefore we cannot simply change it. (In reply to comment #16) > ... > However I still get some coreExceptions in standard error after applying my > fix, the exceptions can not be MessageKind.tooManyIndexProblems exceptions > because they have a nested DOMException exception. See the following error > stack. It is so weird that some coreExceptions (other than > CCorePlugin.STATUS_PDOM_TOO_LARGE and MessageKind.tooManyIndexProblems > exceptions) can still got thrown from parseFile(). Markus, do you have any > observations here? As far as I remember the exception is logged, although it is caught and the indexer continues to work. (In reply to comment #17) > (In reply to comment #14) > > Created an attachment (id=176099) [details] [details] > > 2 fixes applied to cdt.core(see comments for details) > > Find another two problems related to the exceptions we found. > > 1) CCorePlugin.STATUS_PDOM_TOO_LARGE has same value with IStatus.ERROR, they > > are both a status code flag. > > In StandaloneIndexerTask, the function > > protected IStatus createStatus(String msg, Throwable e) { > > return new Status(IStatus.ERROR, "org.eclipse.cdt.core", IStatus.ERROR, > > msg, e); > > } > > create staus using IStatus.ERROR as its code. So the exception status could be > > consider as a CCorePlugin.STATUS_PDOM_TOO_LARGE status. > > Proposed the fis in the patch to chnage CCorePlugin.STATUS_PDOM_TOO_LARGE value > > to 104. > > 2) The function of findOverloadedOperator in class CPPSemantics should check if > > the op2 is null since CPPASTBinaryExpression could return null by its > > getOperand2 function. > > Should anyone have any concern regarding the propsed fix, please let me know. > > Markus, please let me know your comments if you have any. > > Thanks. > IStatus.ERROR is a severity, CCorePlugin.STATUS_PDOM_TOO_LARGE is a status > code, they can have the same value. The method createStatus needs to be > changed. > I know that the specific status code was an unlucky choice (see bug 293021), > however it is public API and therefore we cannot simply change it. > (In reply to comment #16) > > ... > > However I still get some coreExceptions in standard error after applying my > > fix, the exceptions can not be MessageKind.tooManyIndexProblems exceptions > > because they have a nested DOMException exception. See the following error > > stack. It is so weird that some coreExceptions (other than > > CCorePlugin.STATUS_PDOM_TOO_LARGE and MessageKind.tooManyIndexProblems > > exceptions) can still got thrown from parseFile(). Markus, do you have any > > observations here? > As far as I remember the exception is logged, although it is caught and the > indexer continues to work. The createStatus functions in StandaloneIndexerTask override he implementations in PDOMWriter class which do not set IStatus.ERROR as the status code. I am wondering the purpose of this overriding, if it is not necessary I can just remove it and fix the problem. Also, are you OK with my fix in CPPSemantics? > The createStatus functions in StandaloneIndexerTask override he implementations > in PDOMWriter class which do not set IStatus.ERROR as the status code. I am > wondering the purpose of this overriding, if it is not necessary I can just > remove it and fix the problem. I don't think this would work for you. It's sufficient to remove the error code argument. > Also, are you OK with my fix in CPPSemantics? Sure. Ideally you'd also provide a testcase that triggers the issue. This way we can make sure we don't reintroduce the problem in a later release. This bug is used for a bunch of unrelated problems. Please try to use one bug report for one problem, that helps keeping things simpler. (In reply to comment #19) > > The createStatus functions in StandaloneIndexerTask override he implementations > > in PDOMWriter class which do not set IStatus.ERROR as the status code. I am > > wondering the purpose of this overriding, if it is not necessary I can just > > remove it and fix the problem. > I don't think this would work for you. It's sufficient to remove the error code > argument. The createStatus functions in PDMWriiter call the createStatus functions in CCorePlugin, which basically same with the createStatus functions in StandaloneIndexerTask except for the latter has an additional IStatus.Error code argument. So if I remove the overridden functions in StandaloneIndexerTask, it will finally call CCorePlugin's createStatus functions, which will have the same result with your suggestion fix of removing the error code argument. So a duplicate overriding in StandaloneIndexerTask is not necessary but may introduce difficulty to maintain consistency. Let me know if you have a different opinion. Thanks. > > Also, are you OK with my fix in CPPSemantics? > Sure. Ideally you'd also provide a testcase that triggers the issue. This way > we can make sure we don't reintroduce the problem in a later release. > This bug is used for a bunch of unrelated problems. Please try to use one bug > report for one problem, that helps keeping things simpler. OK, good point, I will create separate bugs to resolve these different issues, but first I will collect them here and get agreements from you and the team :-) (In reply to comment #20) > The createStatus functions in PDMWriiter call the createStatus functions in > CCorePlugin, which basically ... > ... So a duplicate overriding in StandaloneIndexerTask is > not necessary but may introduce difficulty to maintain consistency. > Let me know if you have a different opinion. Thanks. I am not really interested in the stand-alone indexer, so from my point of view you can do with it whatever you want. However, I believe that your change won't work for you because the stand-alone indexer runs in an environment where it cannot access all classes, and I think CCorePlugin is a forbidden one, you would need to talk to Chris Recoskie to get more detailed information. (In reply to comment #21) > I am not really interested in the stand-alone indexer, so from my point of view > you can do with it whatever you want. However, I believe that your change won't > work for you because the stand-alone indexer runs in an environment where it > cannot access all classes, and I think CCorePlugin is a forbidden one, you > would need to talk to Chris Recoskie to get more detailed information. What you say is generally true. We have a stubbed out version of CCorePlugin that we bundle along with the standalone indexer though that provides some of its functionality that doesn't require OSGi. So what John is proposing would work, because the hardcoded integer status codes and the method that creates the status objects can be replicated in the stub class without making use of OSGi. (In reply to comment #19) > > The createStatus functions in StandaloneIndexerTask override he implementations > > in PDOMWriter class which do not set IStatus.ERROR as the status code. I am > > wondering the purpose of this overriding, if it is not necessary I can just > > remove it and fix the problem. > I don't think this would work for you. It's sufficient to remove the error code > argument. > > Also, are you OK with my fix in CPPSemantics? > Sure. Ideally you'd also provide a testcase that triggers the issue. This way > we can make sure we don't reintroduce the problem in a later release. > This bug is used for a bunch of unrelated problems. Please try to use one bug > report for one problem, that helps keeping things simpler. I create a separate bug to address CPPSemantics problem, please see the bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=322268 (In reply to comment #21) > (In reply to comment #20) > > The createStatus functions in PDMWriiter call the createStatus functions in > > CCorePlugin, which basically ... > > ... So a duplicate overriding in StandaloneIndexerTask is > > not necessary but may introduce difficulty to maintain consistency. > > Let me know if you have a different opinion. Thanks. > I am not really interested in the stand-alone indexer, so from my point of view > you can do with it whatever you want. However, I believe that your change won't > work for you because the stand-alone indexer runs in an environment where it > cannot access all classes, and I think CCorePlugin is a forbidden one, you > would need to talk to Chris Recoskie to get more detailed information. I create a separate bug for the fix of this problem, https://bugs.eclipse.org/bugs/show_bug.cgi?id=322269 As Chris mentioned, the RDT indexer is fine without these functions since we have a CCorePlugin in the server side, however, I keep the functions of createStatus in StandaloneIndexerTask class ss you suggested, just in case there may be some other applications using it but without CCorePlugin either. (In reply to comment #11) > Created an attachment (id=175889) [details] > a updated patch to be applied to > org.eclipse.cdt.internal.core.dom.parser.cpp.CPPASTAmbiguousTemplateArgument > Thanks, Mike. > Update it for your first two points. Will look at other ambiguous node and > junit test case for them later. Probably need another bugzilla. This is to > address the found LR parser test failures. Create another separate bugzilla, https://bugs.eclipse.org/bugs/show_bug.cgi?id=322426 and add the fix patch to CPPASTAmbiguousTemplateArgument copy function over there. This is fixed. |