Page MenuHomePhabricator

[analyzer] [NFC] PathDiagnostic: Create PathDiagnosticPopUpPiece
AcceptedPublic

Authored by Charusso on Apr 14 2019, 8:55 AM.

Details

Reviewers
NoQ
alexfh
Summary

This new piece is similar to our macro expansion printing in HTML reports:
On mouse-hover event it pops up on variables. Similar to note pieces it
supports plist diagnostics as well.

It is optional, on by default: add-pop-up-notes=true.

Extra: In HTML reports background-color: LemonChiffon was too light,
changed to PaleGoldenRod.

Diff Detail

Event Timeline

Charusso created this revision.Apr 14 2019, 8:55 AM
NoQ added a comment.Apr 16 2019, 3:33 PM

In HTML reports background-color: LemonChiffon was too light, changed to PaleGoldenRod.

Both look good to me, though i'd easily believe that LemonChiffon is indeed indistinguishable from white on some monitors with certain brightness/contrast settings.

clang/lib/Rewrite/HTMLRewrite.cpp
329

Blue over yellow looks a bit acid, maybe use a... "bluer"?... background for our pop-ups? Like, both for the markup and for the actual popup.

440–442

Why is it necessary to re-lex everything in this case? I don't really know why this was necessary for macros, but in this case i'm more confused. The popup piece is supposed to point us to a specific source range; why can't we just highlight that source range directly?

NoQ added a reviewer: alexfh.Apr 16 2019, 3:52 PM
NoQ added a subscriber: alexfh.

Let's decide whether we want to display the new notes in the text output. @alexfh: Given that clang-tidy is the primary consumer of the analyzer's text output mode, would you rather have or rather not have these new note:s in the text output? (scroll to the bottom of D53076 to see how they look). In my opinion, i'd rather have them in text output (because text output is hard to navigate, and having the necessary information exactly where you need it would probably be helpful), but rather not have them in tools that parse clang-tidy notes and stuff them into a source code view in some GUI (such tools do exist, right?), so im confused. I'd also prefer to have them be off in text output in tests, because such tests usually test checker-specific notes and having more checker-inspecific notes in the tests is more annoying than useful. But that's pretty minor; i'll be happy to write them down every time i write a test if they are believed to be a good thing in text output in general.

In D60670#1469494, @NoQ wrote:

Let's decide whether we want to display the new notes in the text output. @alexfh: Given that clang-tidy is the primary consumer of the analyzer's text output mode, would you rather have or rather not have these new note:s in the text output? (scroll to the bottom of D53076 to see how they look). In my opinion, i'd rather have them in text output (because text output is hard to navigate, and having the necessary information exactly where you need it would probably be helpful), but rather not have them in tools that parse clang-tidy notes and stuff them into a source code view in some GUI (such tools do exist, right?), so im confused. I'd also prefer to have them be off in text output in tests, because such tests usually test checker-specific notes and having more checker-inspecific notes in the tests is more annoying than useful. But that's pretty minor; i'll be happy to write them down every time i write a test if they are believed to be a good thing in text output in general.

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

  • clang-format PlistDiagnostics.cpp

I wanted to do this for the longest time, but @george.karpenkov was very much against it. Is this desired? D52735#1251344

Charusso planned changes to this revision.Mon, Apr 22, 10:07 AM
Charusso marked 3 inline comments as done.

Thanks!

@Szelethus, I will revert it.

In D60670#1469494, @NoQ wrote:

Let's decide whether we want to display the new notes in the text output.

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

@alexfh, add them as a Clang option with -f? I do not know how Clang Tidy would use that, so that I am not sure. I would create an AnalyzerOptions option as I see that far only.

clang/lib/Rewrite/HTMLRewrite.cpp
329

I am planning to change the entire CSS business as the current representation already too acid. Do you mean to change the PaleGoldenRod to blue on conditions as a new kind of highlight?

440–442

I just did not know about PathDiagnosticRange.

Can we add an option to enable or disable these notes similar to Clang's -fdiagnostics-show-option, -fdiagnostics-show-note-include-stack, etc?

@alexfh, add them as a Clang option with -f? I do not know how Clang Tidy would use that, so that I am not sure. I would create an AnalyzerOptions option as I see that far only.

I gave Clang's -f options as an example only. An AnalyzerOptions option would work too. Just something we can change if needed.

Charusso updated this revision to Diff 196422.Wed, Apr 24, 4:45 AM
Charusso marked an inline comment as done.
Charusso edited the summary of this revision. (Show Details)
Charusso added subscribers: gsd, gerazo.
  • add-pop-up-notes=true
  • Revert clang-format.

Feature request test is in D61060.

Now it is blue and the highlight is as wide as the condition:

Before:

NoQ added a comment.Wed, Apr 24, 5:34 PM

Hmm, i just realized that there may be multiple pop-ups for the same expression, with different values. This may happen if the expression is used in a loop or the function is called multiple times. We'd have to include all of them somehow, maybe something like this:

   /------------------\
   | (42.a)  X is 5   |    /-----------------\
   | (69.a)  X is -2  |    | (42.b)  Y is 7  |
   \  /---------------/    \  /--------------/
    \/                      \/

if (X >= 0        &&        Y <= 10) {

  /-------------------------\
  | (43) Taking true branch |
  \-------------------------/
  /--------------------------\
  | (70) Taking false branch |
  \--------------------------/

}
Charusso updated this revision to Diff 197000.Sat, Apr 27, 7:55 PM
Charusso marked 2 inline comments as done.

Fix multiple reports at the same range:

  • Plain text became a table.
  • Styling is the same as the bubbles.
  • mrange (whatever it means) - the grey highlight cannot overlap pop-up ranges.
Charusso added a comment.EditedSat, Apr 27, 8:02 PM

Your indexing idea has a problem: zero index would be possible and usually you would compare the gray PathDiagnostic bubbles with these notes which index would get an offset by one.

At line 723 there would be a zero index, which is confusing, and at line 1052 you could see the new approach:

NoQ added a comment.Wed, May 1, 7:12 PM

Wonderful!!

The reason why i don't want numbers on pop-up notes match numbers on regular notes is that the text on the pop-up note may no longer be true at the moment of time when we reach the regular note. Like, the value of the variable may change in between.

We can do one of the following:

  • "42.a", "42.b", ... (still confusing as it if's part of 42);
  • "42½", "42⅓", "42⅔", ..., just give up and drop them when there more than 10 popups between two events specific, because that's how many unicode vulgar fractions we have;
  • Just "4243" on all pop-ups (indicates that this is somewhere between 42 and 43).

If it turns out to be still confusing, i'd recommend committing it as off-by-default until we come up with a better UI. 'Cause the work is wonderful regardless.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
208

Let's specify "pop-up notes", because "extra notes" refer to the note pieces.

Charusso updated this revision to Diff 197698.Wed, May 1, 8:21 PM
Charusso marked 2 inline comments as done.

index → index+1

Thanks for the review!

I have not seen problematic indexing, that is why I was not sure about your idea. If someone ever find confusing stuff I would change it immediately to off by default.

Final look:

PS: That offset-by-one index + arrow confuse me more, so that I am not sure about that improvement being a useful one.

NoQ added a comment.Mon, May 6, 6:45 PM

Could you have a quick look at how does the vulgar fractions solution look?, i.e., 42½ etc., maybe you can modify the html directly if you don't want to code it, as it might be annoying to code. I don't immediately see any downsides.

Charusso updated this revision to Diff 198633.Wed, May 8, 5:49 AM
  • Sub-state indexing.

I have not got better idea to just index over each sub-state. Example at line 1052:

PS: I do not like that fraction stuff as it is difficult to read, ugly and limited.

@NoQ could you share your final thoughts here, please? I would like to move forward and never look back.

NoQ added a comment.Fri, May 17, 2:58 PM

Mmm, just had a fresh look and i kinda like it. I think in its current shape it's intuitive enough what this means.

Let's make sure that all popups have the extra .digit in them and commit.

Charusso updated this revision to Diff 200109.Fri, May 17, 3:36 PM
  • Add extra state transition indices to every pop-up note.

Thanks you! I hope this feature will be useful to see what is behind the scenes and create better reports.

Usage could be found at line 2041 with line 2129:

PS: I have no idea why the macro note is purple, but because the entire HTML printing is broken I hope you saw nothing.

NoQ accepted this revision.Fri, May 17, 3:53 PM

Go for it!

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
698–699

This comment looks outdated.

This revision is now accepted and ready to land.Fri, May 17, 3:53 PM