Bugzilla – Full Text Bug Listing
|
Description
Lars Vogel
New Gerrit change created: https://git.eclipse.org/r/96878 New Gerrit change created: https://git.eclipse.org/r/96879 New Gerrit change created: https://git.eclipse.org/r/96880 New Gerrit change created: https://git.eclipse.org/r/96881 New Gerrit change created: https://git.eclipse.org/r/96882 New Gerrit change created: https://git.eclipse.org/r/96883 New Gerrit change created: https://git.eclipse.org/r/96884 New Gerrit change created: https://git.eclipse.org/r/96885 In general i think it is ok - i briefly looked at the cases although and think in most of them the jit optimizes away the locks because it understands the single thread nature (In reply to Thomas Schindl from comment #9) > In general i think it is ok - i briefly looked at the cases although and > think in most of them the jit optimizes away the locks because it > understands the single thread nature AFAIK JIT will only get active after a relative high number of invocations. Therefore using the faster API should give us some speed improvements. I've already commented this on JDT bug and I would simply repeat it here: Lars, please, this makes no sense to change for tests, except you wastes your and other time and makes the history changes that hide the real ones. I would also say that mass change for not performance critical production code is conter-productive and will only disturb others from work and make tracking history changes impossible. A big -1 from me. No mass changes, and only change in places where we know about performance bottle necks caused by StringBuffer. (In reply to Andrey Loskutov from comment #11) See my reply in Bug 516844. I have warnings in my code for this as I'm using SonarLint. Also AFAIK StringBuilder should be preferred compared to StringBuilder in code and updating our code basis to modern constructs is IMHO important. (In reply to Lars Vogel from comment #12) > (In reply to Andrey Loskutov from comment #11) > > See my reply in Bug 516844. I have warnings in my code for this as I'm using > SonarLint. Also AFAIK StringBuilder should be preferred compared to > StringBuilder in code and updating our code basis to modern constructs is > IMHO important. Andrey, remember the unnecessary trailing whitespace issues which affected you, which we solved with mass changes? (In reply to Lars Vogel from comment #13) > (In reply to Lars Vogel from comment #12) > > (In reply to Andrey Loskutov from comment #11) > > > > See my reply in Bug 516844. I have warnings in my code for this as I'm using > > SonarLint. Also AFAIK StringBuilder should be preferred compared to > > StringBuilder in code and updating our code basis to modern constructs is > > IMHO important. > > Andrey, remember the unnecessary trailing whitespace issues which affected > you, which we solved with mass changes? The difference is that Git can ignore white space changes, but can't do this for anything else. This is a big issue if you are trying undrestand the reason behind the otherwise undocumented code. (In reply to Andrey Loskutov from comment #14) > The difference is that Git can ignore white space changes, but can't do this > for anything else. This is a big issue if you are trying undrestand the > reason behind the otherwise undocumented code. Take a look at some of the changed lines in my Gerrit. Most of them are StringBuilder/StringBuffer var = new StringBuilder/StringBuffer. I doubt that you will use these lines to identify bugs. (In reply to Lars Vogel from comment #15) > (In reply to Andrey Loskutov from comment #14) > > > The difference is that Git can ignore white space changes, but can't do this > > for anything else. This is a big issue if you are trying undrestand the > > reason behind the otherwise undocumented code. > > Take a look at some of the changed lines in my Gerrit. Most of them are > StringBuilder/StringBuffer var = new StringBuilder/StringBuffer. I doubt > that you will use these lines to identify bugs. Agree, if you can guarantee that the patches only touch cases where only one line with default StringBuffer constructor without arguments is affected (e.g. not a nested call as argument to another function etc). So StringBuffer var = new StringBuffer() is OK, but NOT for StringBuffer var = new StringBuffer(42) or callMe(new StringBuffer("42")) etc (In reply to Andrey Loskutov from comment #16) > So StringBuffer var = new StringBuffer() is OK, but NOT for > StringBuffer var = new StringBuffer(42) or callMe(new StringBuffer("42")) etc Sounds like a good starting point. Lets use this bug for this work and revisit the more complex work with more complex bug reports. New Gerrit change created: https://git.eclipse.org/r/98760 New Gerrit change created: https://git.eclipse.org/r/98773 New Gerrit change created: https://git.eclipse.org/r/98838 Gerrit change https://git.eclipse.org/r/98773 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6bab612a4fd7a00332afe4412441db9ad0b15b36 Gerrit change https://git.eclipse.org/r/96878 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=40f668c37e090b537aeaf67456b348ba96ce690c Gerrit change https://git.eclipse.org/r/98838 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=a2def5476b0222c7fee40a304fee36f0d29119ee Gerrit change https://git.eclipse.org/r/96879 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c42709d2e2893a355d9c654f67301dc349e56ef0 Gerrit change https://git.eclipse.org/r/96880 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=da2e408f6ecdeb2019c2298b4372097d60dc7c92 Gerrit change https://git.eclipse.org/r/96883 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=10e998f40b0111726cf2825cf6876d59d5f47025 Gerrit change https://git.eclipse.org/r/96884 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3576da2e8ef94ed2c06ccf4131f66fef603accb5 Gerrit change https://git.eclipse.org/r/96885 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=cf36064add4e55307816544b05d0d230aeaefcad All patches are in can we close it? (In reply to Alexander Kurtakov from comment #29) > All patches are in can we close it? Thanks, Alex for the review and merges. Gerrit change https://git.eclipse.org/r/101138 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2a1936a0f33c26c9939fcd17184a709e4177b675 |