Menu
▾
▴
[Cdk-devel] [ cdk-Patches-2802784 ] Some solved threading issues
|
From: SourceForge.net <no...@so...> - 2010-10-16 21:04:56
|
Patches item #2802784, was opened at 2009-06-08 04:10 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2802784&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: Needs Revision Status: Open Resolution: None Priority: 5 Private: No Submitted By: Thorsten Meinl (sithmein) Assigned to: Nobody/Anonymous (nobody) Summary: Some solved threading issues Initial Comment: This patch contains some fixes Jonathan and I made at the CDK workshop in Cambridge. Its mostly tagging classes/methods as thread safe (or not), fixing some thread-related issues, fixing some other issues and also some minor code cleanup. ---------------------------------------------------------------------- >Comment By: Rajarshi Guha (rajarshi) Date: 2010-10-16 17:04 Message: Any updates on this? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-07-22 07:08 Message: OK, the patch needs updating for the annotation tags introduced in: http://cdk.git.sourceforge.net/git/gitweb.cgi?p=cdk;a=commit;h=2e562a2434a7214ca06595d51e2ec467e1791be5 *instead* of lines in the JavaDoc. I also like to see the use of final explained (not necessarily in the patch, but perhaps as separate patch), what role it has in thread safety... this could potentially lead to a unit test... I also note JavaDoc stubs like: + * @param atomContainer + * @return + * @throws CDKException which interrupt the JavaDoc quality checking; please either: 1) remove such template JavaDoc, 2) complete that it. I also second Rajarshi's comment that changes like: - if (singleRing.getAtomCount() > maxRingSize) maxRingSize = singleRing.getAtomCount(); + if (singleRing.getAtomCount() > maxRingSize) { + maxRingSize = singleRing.getAtomCount(); + } are not necessarily needed, and actually make it much more work to review patches, as I now have to decide for the above if is is just style/layout or a secretly sneaking in code changes... I guess not, but I will have to check that one by one... ---------------------------------------------------------------------- Comment By: Thorsten Meinl (sithmein) Date: 2009-06-10 01:57 Message: I also favor the use of special Javadoc-tags. Regarding the formatting: that is certainly nothing that needs to be changed. These changes came from the auto-formatting we use in Eclipse for KNIME. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-06-09 22:09 Message: Patch looks OK, however I think it'd be a good idea to use annotations/tags rather than free text. So methods that are deemed threadsafe should have a Javadoc tag @threadsafe and those that are not would have @notthreadsafe This way a taglet can be written to generate a summary report, allowing devs to keep track of what has and has not been checked. Also I don't see the point of stuff such as - if (type == null) type = perceiveHalogens(atomContainer, atom); - if (type == null) type = perceiveCommonSalts(atomContainer, atom); - if (type == null) type = perceiveOrganometallicCenters(atomContainer, atom); - if (type == null) type = perceiveNobelGases(atomContainer, atom); + if (type == null) { + type = perceiveHalogens(atomContainer, atom); + } + if (type == null) { + type = perceiveCommonSalts(atomContainer, atom); + } + if (type == null) { + type = perceiveOrganometallicCenters(atomContainer, atom); + } + if (type == null) { + type = perceiveNobelGases(atomContainer, atom); + } The previous version (-) was pretty concise - why change it to the expanded version? I realize that this is generally a personal preference, but unless it is egregiously unformatted, I don't see a need to change it ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-06-09 11:21 Message: I see no problems with that. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2802784&group_id=20024 |