This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Aug 1 2019, 7:11 AM
NoQ added a comment.Aug 1 2019, 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.Aug 12 2019, 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.

NoQ added a comment.Aug 20 2019, 12:28 PM

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.

I guess let's just land that variant! I'm also not super worried about "The" here.

Szelethus updated this revision to Diff 216273.Aug 20 2019, 3:45 PM
  • Change the condition postfix as discussed
  • expected-note{{...}} -> expected-note-re{{{{^}}...{{$}}}}
NoQ accepted this revision.Aug 20 2019, 3:54 PM

Thanks!!

I also suspect we don't need the comma.

This revision is now accepted and ready to land.Aug 20 2019, 3:54 PM
Szelethus added a comment.EditedAug 20 2019, 4:11 PM

Hmm-hmm, I kinda like the comma, but would happily concede on this. Its been a while since my english exam :^)

LGTM! I think the UIs could do better displaying this info in the future but this is not your job :)

xazax.hun accepted this revision.Aug 20 2019, 4:14 PM

*actually accepting*

LGTM! I think the UIs could do better displaying this info in the future but this is not your job :)

https://github.com/Ericsson/codechecker/pull/2279

CodeChecker now indents function calls! Woohoo!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 1:42 PM