Bugzilla – Full Text Bug Listing
| Summary: | ASTNode.copySubtree(): NullPointerException if name or type is not set | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] JSDT | Reporter: | Etienne Pfister <epfister> | ||||||
| Component: | General | Assignee: | Phil Berkland <berkland> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Phil Berkland <berkland> | ||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | cbachman, hjzhang, kaloyan | ||||||
| Version: | unspecified | Keywords: | contributed | ||||||
| Target Milestone: | 3.0 RC1 | Flags: | berkland:
pmc_approved?
(david_williams) berkland: pmc_approved? (raghunathan.srinivasan) berkland: pmc_approved? (naci.dai) berkland: pmc_approved? (deboer) berkland: pmc_approved? (neil.hauge) kaloyan: pmc_approved+ |
||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | PMC_approved | ||||||||
| Attachments: |
|
||||||||
This patch looks good. Patch looks trivial, just a null check added.
But, I have a comment on the patch implementation. In my opinion the pattern:
if (getSomething() != null) {
doSomethingWith(getSomething());
}
is a bad practice. Especially, if getSomething() does something more than just returning an object reference. This is a precondition for possible performance and synchronization issues. And, as I look in the code, the getName() and getType() methods looks they were quite complex in the past (the commented code). Nothing can guarantee that their implementation may become complex again in the future. Therefore, I propose to use the following safer pattern:
Something something = getSomething();
if (something != null) {
doSomethingWith(something);
}
Please, also add "PMC" in Whiteboard and Helen in CC as specified in the defect review guidelines:
http://wiki.eclipse.org/WTP_PMC_Defect_Review
The Target Milestone field should be set appropriately, too.
Another thing... Is Etienne a committer? If not, you should add the "contribution" keyword in the Keywords field: http://dev.eclipse.org/mhonarc/lists/wtp-dev/msg06019.html Created attachment 99603 [details]
New Patch to solve copySubtree issue
adapted patch according to Kaloyan Raev
Approved Applied patch, thank you for your contribution. |
Created attachment 99271 [details] Patch to solve copySubtree issue In the method ASTNode.copySubtree() if the tree to copy contains one of the following node with the given condition a NullPointerException will be thrown in the clone0() method of these classes. FunctionDeclaration: If attribute methodName (getName()) is not set ClassInstanceCreation: If attribute type (getType()) is not set In both cases the default value is null. The problem occurs because JavaScript allows function declarations without a name and JavaScript has no types. The included patch fixes these two issues to make ASTNode.copySubtree() work.