Bugzilla – Full Text Bug Listing
| Summary: | Use datatypes suitable to express the requirements of an api | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Carsten Hammer <carsten.hammer> |
| Component: | UI | Assignee: | Carsten Hammer <carsten.hammer> |
| Status: | CLOSED WONTFIX | QA Contact: | Fabrice Tiercelin <fabrice.tiercelin> |
| Severity: | normal | ||
| Priority: | P3 | CC: | fabrice.tiercelin, jjohnstn |
| Version: | 4.19 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/172988 https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/172260 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=71c57cdeedcac3266f60e455d05d89bc1b5be564 https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174577 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=45c8db5b5398dcccef411a6137c6db4028a33119 |
||
| Whiteboard: | |||
|
Description
Carsten Hammer
One other thing to consider is that git history gets replaced and this has bearing in future maintenance. If the changes correct errors, enable some feature, or have noticeable performance benefits, then by all means they are worth doing. Otherwise, if they are just a rewrite of code, especially if the code has been stable for some time, I am not sure of the benefit. (In reply to Jeff Johnston from comment #1) > One other thing to consider is that git history gets replaced and this has > bearing in future maintenance. If the changes correct errors, enable some > feature, or have noticeable performance benefits, then by all means they are > worth doing. Otherwise, if they are just a rewrite of code, especially if > the code has been stable for some time, I am not sure of the benefit. True, would it be OK to check one or both of the attached gerrits for being acceptable under this consideration or not or should I close this buzilla and abandon its gerrits directly? Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/172260 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=71c57cdeedcac3266f60e455d05d89bc1b5be564 (In reply to Carsten Hammer from comment #2) > (In reply to Jeff Johnston from comment #1) > > One other thing to consider is that git history gets replaced and this has > > bearing in future maintenance. If the changes correct errors, enable some > > feature, or have noticeable performance benefits, then by all means they are > > worth doing. Otherwise, if they are just a rewrite of code, especially if > > the code has been stable for some time, I am not sure of the benefit. > > True, would it be OK to check one or both of the attached gerrits for being > acceptable under this consideration or not or should I close this buzilla > and abandon its gerrits directly? Regarding the first gerrit, I would recommend dropping it as one change is quite large and others are on tests which unless they create noticeable performance gains, probably don't justify the loss of annotation history. Regarding the gerrit that was just merged below which only had a small amount of chnages: can you guarantee that the fSettings will never be accessed via multiple threads and therefore the code should use the original Hashtable? If not, then it should likely be reverted, otherwise, I am ok as it touches few lines. (In reply to Jeff Johnston from comment #4) > (In reply to Carsten Hammer from comment #2) > > (In reply to Jeff Johnston from comment #1) > > > One other thing to consider is that git history gets replaced and this has > > > bearing in future maintenance. If the changes correct errors, enable some > > > feature, or have noticeable performance benefits, then by all means they are > > > worth doing. Otherwise, if they are just a rewrite of code, especially if > > > the code has been stable for some time, I am not sure of the benefit. > > > > True, would it be OK to check one or both of the attached gerrits for being > > acceptable under this consideration or not or should I close this buzilla > > and abandon its gerrits directly? > > Regarding the first gerrit, I would recommend dropping it as one change is > quite large and others are on tests which unless they create noticeable > performance gains, probably don't justify the loss of annotation history. > > Regarding the gerrit that was just merged below which only had a small > amount of chnages: can you guarantee that the fSettings will never be > accessed via multiple threads and therefore the code should use the original > Hashtable? If not, then it should likely be reverted, otherwise, I am ok as > it touches few lines. Hashtable is not only a question of using a synchronized collection. This type as is is deprecated and there are replacements that offer synchronization for a reason. We can change to a synchronized collection without making use of Hashtable. Ok, I thought that the design changes and its benefits for new possibilities regarding parallel stream processing, type safety, interface design and future maintenance are so obvious that it outweighs arguments to „never change a running system“. That’s obviously not the case. Then let‘s revert and after that close as „wontfix“.. Maybe we can pick this up at a later point in time. New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174577 Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/174577 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=45c8db5b5398dcccef411a6137c6db4028a33119 (In reply to Carsten Hammer from comment #5) > (In reply to Jeff Johnston from comment #4) > > (In reply to Carsten Hammer from comment #2) > > > (In reply to Jeff Johnston from comment #1) > > > > One other thing to consider is that git history gets replaced and this has > > > > bearing in future maintenance. If the changes correct errors, enable some > > > > feature, or have noticeable performance benefits, then by all means they are > > > > worth doing. Otherwise, if they are just a rewrite of code, especially if > > > > the code has been stable for some time, I am not sure of the benefit. > > > > > > True, would it be OK to check one or both of the attached gerrits for being > > > acceptable under this consideration or not or should I close this buzilla > > > and abandon its gerrits directly? > > > > Regarding the first gerrit, I would recommend dropping it as one change is > > quite large and others are on tests which unless they create noticeable > > performance gains, probably don't justify the loss of annotation history. > > > > Regarding the gerrit that was just merged below which only had a small > > amount of chnages: can you guarantee that the fSettings will never be > > accessed via multiple threads and therefore the code should use the original > > Hashtable? If not, then it should likely be reverted, otherwise, I am ok as > > it touches few lines. > > Hashtable is not only a question of using a synchronized collection. This > type as is is deprecated and there are replacements that offer > synchronization for a reason. We can change to a synchronized collection > without making use of Hashtable. > > Ok, I thought that the design changes and its benefits for new possibilities > regarding parallel stream processing, type safety, interface design and > future maintenance are so obvious that it outweighs arguments to „never > change a running system“. That’s obviously not the case. Then let‘s revert > and after that close as „wontfix“.. > > Maybe we can pick this up at a later point in time. Thanks Carsten. |