Page MenuHomePhabricator

[analyzer] Mention whether an event is about a condition in a bug report part 1
Needs ReviewPublic

Authored by Szelethus on Thu, Aug 1, 7:11 AM.

Details

Summary

Can't add much more to the title! This is part 1, the case where the collapse point isn't in the condition point is the responsibility of ConditionBRVisitor, which I'm addressing in part 2.

Diff Detail

Event Timeline

Szelethus created this revision.Thu, Aug 1, 7:11 AM
NoQ added a comment.Thu, Aug 1, 1:36 PM

Fantastic! Let's open the wording bikeshed season?

I suspect that a simple "(The) Value -> Condition value" change would have worked better.

Another variant: "Value ..., which participates in a condition later".

In D65575#1611013, @NoQ wrote:

Fantastic! Let's open the wording bikeshed season?

I suspect that a simple "(The) Value -> Condition value" change would have worked better.

Another variant: "Value ..., which participates in a condition later".

Yea, I kinda prefer a more uniform indication as to whether we're explaining a condition or "THE value". While I personally took a unique approach in evaluating analysis results (my eye was hunting for the changes I made specifically), I did find each function call in the bug report super easy to understand:


See how this function call screams what it is about? Now, condition tracking is inherently imperfect (like bug report construction as a whole), and whenever I feel like the notes added by it provide little value, simply glancing at the notes can tell whether I should observe that function call or not.

I think it isn't crucial of getting rid of the "The" prefix, if we append ", which participates in a condition later" (which sounds so much better than what I added in this patch), so maybe changing WillBeUsedForACondition to that would be good enough. However, as I said, I realize that the way I looked at these results was a lot different than how the average user will do so, so I'm totally open on this topic.

Yeah, this is important bike-shedding from the user point of view. Otherwise it looks good to me.

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

NoQ added a comment.Mon, Aug 12, 12:52 PM

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

Mmm, what are your thoughts on my suggestions with wording? I'll also poke Devin more.

Also it looks like most of the bottom of the stack is already accepted, you can just commit most of it, right?

In D65575#1625741, @NoQ wrote:

A little early, gentle ping :) I'm getting kinda paranoid with the size of the stack, and how much I struggled with commiting last time.

Mmm, what are your thoughts on my suggestions with wording? I'll also poke Devin more.

Pretty much this:

I think it isn't crucial of getting rid of the "The" prefix, if we append ", which participates in a condition later" (which sounds so much better than what I added in this patch), so maybe changing WillBeUsedForACondition to that would be good enough. However, as I said, I realize that the way I looked at these results was a lot different than how the average user will do so, so I'm totally open on this topic.

I figured we'd have another round on this, but if it sounds good, I'll update the patch.

Also it looks like most of the bottom of the stack is already accepted, you can just commit most of it, right?

Actually yea, I had a good reason to delay but not anymore.