Re: svn commit: r1817750 - in /ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/ base/component: ComponentConfig.java ComponentLoaderConfig.java ComponentResourceHandler.java

[Michael Brohl]

I've reverted these commits and reapplied the original patches.

Thanks all for taking care,

Michael


Am 11.12.17 um 15:21 schrieb Jacques Le Roux:

I reviewed r1817748 1817742 1817743 1817744 there are OK with me



Notably beware of

"if (debug) {" in r1817748.

Jacques


Le 11/12/2017 à 12:39, Michael Brohl a écrit :

So you just have to revert the removing of the tests on Debug in these commits  (IIRW only in r1817748 and r1817750)

Thanks, Taher! :-)

Am 11.12.17 um 11:50 schrieb Taher Alkhateeb:

On a side note to Michael kudos on the massive bug hunting effort lately.

On Dec 11, 2017 1:33 PM, "Michael Brohl" <michael.br...@ecomify.de> wrote:

I will see how I can handle this effectively without doing all my review

work twice.

There should be not too much commits which are affected.

I'll see if I can repair it this evening.


Am 11.12.17 um 11:29 schrieb Jacques Le Roux:

Michael,


1817742 1817743 1817744 (the bigger ones ;))

Thanks

Jacques


Le 11/12/2017 à 10:21, Michael Brohl a écrit :

It would be easier for reviewers if you could revert your commits (not those of the weekend, I have not reviewed them all yet but most of them, and so far did not find any important issues, just few improvements I did). This would prevent us from double reviewing. For me at least r1817748 and

These are good points, I wasn't aware of this and just saw the (in my

view) unnecessary extra lines of code.

I will check my latest commits and change this back where it is
reasonable (i.e. where we have concatenations).

Luckily, I just started this this morning and not during the heavier
commits over the weekend.


the commits of patches, it's too confusing.

Thanks,

Michael


Am 11.12.17 um 09:55 schrieb Jacopo Cappellato:

This reminds me that it is NOT a good idea to do such changes along with

On Mon, Dec 11, 2017 at 9:31 AM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:

[...]

Also for OFBIZ-9872 the Jira lacks a description to possibly understand

why theses changes (notably why you rightly removed the "if

"if
(isOn(level)) {"

There is actually a valid reason for maintaining the pattern:

(Debug.verboseOn())" test because it's done at the bottom level with

if (Debug.verboseOn()) {
      Debug.logVerbose(string + concatenation + here);
}


(computational, memory, garbage collection); since the string
concatenation

will

before: if the call returns a false then at runtime the sting
concatenation
will not be executed at all.

Jacopo

In fact string concatenation (in any of its forms) adds some overhead is done before calling Debug.logVerbose (or similar methods), but it is only useful if that log level is on (otherwise the call to logVerbose not produce any output) then it is wise to perform the verboseOn() check

smime.p7s
Description: S/MIME Cryptographic Signature