QA rule suggestion - assignment symbol slips
A quick suggestion for anyone working on QA rules... Just now I am playing with FFI a bit and switching back a forth from Pharo to C and sometimes I end up doing... location = Libclang clang_getRangeStart__range: range. thus QA reported a. "Temporaries read before written" and b. "Temporary variables not read or not written" but these weren't helpful since I was stuck staring at it saying to myself "Well range is assigned before its used, and location is assigned, so bah! the rules seem broken" and for about 30 seconds I remained blind to the truth I'd left the colon off the assignment operator. I'd expect newcomers from other languages might slip in the same way, so a higher priority rule extending [b.] that identifies such variable followed by equal-sign would provide pertinent feedback. cheers -ben
Hi, The "Temporaries read before writtenâ rule is implemented very in a way that you get *a lot* of false positives. e.g. it treats *any* block as potentially no exectuted, so any code like messageSelectorAndArgumentNames | t | self doSomething: [ t:=1 ]. ^t or messageSelectorAndArgumentNames | t | self init: t. ^t triggers the rule. There are many false positives. I sometimes wonder if this rule makes sense with this strict interpretation. It leads to bug tracker entries like this: https://pharo.fogbugz.com/f/cases/18943/PositionableStream-upTo-raised-temp-... Marcus
On 28 Nov 2017, at 02:54, Ben Coman <btc@openinworld.com> wrote:
A quick suggestion for anyone working on QA rules...
Just now I am playing with FFI a bit and switching back a forth from Pharo to C and sometimes I end up doing... location = Libclang clang_getRangeStart__range: range.
thus QA reported a. "Temporaries read before written" and b. "Temporary variables not read or not written" but these weren't helpful since I was stuck staring at it saying to myself "Well range is assigned before its used, and location is assigned, so bah! the rules seem broken" and for about 30 seconds I remained blind to the truth I'd left the colon off the assignment operator.
I'd expect newcomers from other languages might slip in the same way, so a higher priority rule extending [b.] that identifies such variable followed by equal-sign would provide pertinent feedback.
cheers -ben
as for catching message sends of the kind var = 1 + 2. (instead of :=), yes, we can add a rule for that.
On 28 Nov 2017, at 09:20, Marcus Denker <marcus.denker@inria.fr> wrote:
Hi,
The "Temporaries read before writtenâ rule is implemented very in a way that you get *a lot* of false positives.
e.g. it treats *any* block as potentially no exectuted, so any code like
messageSelectorAndArgumentNames | t | self doSomething: [ t:=1 ]. ^t
or
messageSelectorAndArgumentNames | t | self init: t. ^t
triggers the rule. There are many false positives.
I sometimes wonder if this rule makes sense with this strict interpretation.
It leads to bug tracker entries like this:
https://pharo.fogbugz.com/f/cases/18943/PositionableStream-upTo-raised-temp-...
Marcus
On 28 Nov 2017, at 02:54, Ben Coman <btc@openinworld.com> wrote:
A quick suggestion for anyone working on QA rules...
Just now I am playing with FFI a bit and switching back a forth from Pharo to C and sometimes I end up doing... location = Libclang clang_getRangeStart__range: range.
thus QA reported a. "Temporaries read before written" and b. "Temporary variables not read or not written" but these weren't helpful since I was stuck staring at it saying to myself "Well range is assigned before its used, and location is assigned, so bah! the rules seem broken" and for about 30 seconds I remained blind to the truth I'd left the colon off the assignment operator.
I'd expect newcomers from other languages might slip in the same way, so a higher priority rule extending [b.] that identifies such variable followed by equal-sign would provide pertinent feedback.
cheers -ben
On 28 November 2017 at 16:25, Marcus Denker <marcus.denker@inria.fr> wrote:
as for catching message sends of the kind
var = 1 + 2.
(instead of :=), yes, we can add a rule for that.
Cool. I added https://pharo.fogbugz.com/f/cases/20780
On 28 Nov 2017, at 09:20, Marcus Denker <marcus.denker@inria.fr> wrote:
Hi,
The "Temporaries read before writtenâ rule is implemented very in a way that you get *a lot* of false positives.
e.g. it treats *any* block as potentially no exectuted, so any code like
messageSelectorAndArgumentNames | t | self doSomething: [ t:=1 ]. ^t
or
messageSelectorAndArgumentNames | t | self init: t. ^t
triggers the rule. There are many false positives.
I sometimes wonder if this rule makes sense with this strict interpretation.
It leads to bug tracker entries like this:
https://pharo.fogbugz.com/f/cases/18943/PositionableStream-upTo-raised-temp-...
Marcus
On 28 Nov 2017, at 02:54, Ben Coman <btc@openinworld.com> wrote:
A quick suggestion for anyone working on QA rules...
Just now I am playing with FFI a bit and switching back a forth from Pharo to C and sometimes I end up doing... location = Libclang clang_getRangeStart__range: range.
thus QA reported a. "Temporaries read before written" and b. "Temporary variables not read or not written" but these weren't helpful since I was stuck staring at it saying to myself "Well range is assigned before its used, and location is assigned, so bah! the rules seem broken" and for about 30 seconds I remained blind to the truth I'd left the colon off the assignment operator.
I'd expect newcomers from other languages might slip in the same way, so a higher priority rule extending [b.] that identifies such variable followed by equal-sign would provide pertinent feedback.
cheers -ben
participants (2)
-
Ben Coman -
Marcus Denker