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

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

• Add extra state transition indices to every pop-up note.

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

PS: Outcome of program state printing could be found in D62087 which is very similar to your idea in D61677.

Thanks for the accept! Sorry for the extra noise but it came out it is tricky to create valid JSON for each sub-print and we do not need it for now. I wanted to create the most compact and necessary JSON without touching the JsonSupport.h.

• Better header handling.
• Less JSON.

I do not like to have a variable storing/mention one stuff named in plural. It is your decision as it is just my personal feeling.

PrettyStackTraceLocationContext.h definitely needs the default print() method (may other users too). Even it is a little bit silly but I think we have to keep them and create printJSON(). What do you think?

• More JSONify.

What about "the most derived class" or "a superclass" instead of "the superclass"? (https://en.cppreference.com/w/cpp/language/derived_class)
May the sentence is a little bit too long. It would be cool to say "by A" or something more simple and precise.

PS: I mean we do not write it out and I am not sure whether it is the first/last of the ClusterBindings or what.

In D61677#1497130, @NoQ wrote:

{ "trait" : "Store", "items" : [
  { "cluster" : "SymRegion{reg_$2<x>}", "items" : [
    { "kind" : "Direct", "offset" : 0, "value" : "reg_$0<a> + reg_$1<b>" },

Abandoned like its parent revision.

In D61677#1497292, @NoQ wrote:

I propose neither implementing ProgramState::printJSON() nor ProgramState::printDiff() but instead replacing the current implementation of ProgramState::print()
Then i propose making a tool that replaces JSON curly braces with GraphViz's HTML tags for prettier looks.

I see, now I get it. Thanks for the idea! Sadly Graphviz has a very limited HTML usage so this tool has to do a lot more. I like that idea to have machine code inside the graph so I can parse it and modify it whatever I like without any extra overhead of very complex LLVM file-handling/command execution. I will mention the source of these ideas in the actual patch.

In addition: If I am right you would use the CTU-level more complex JSON format with tons of additional stuff compared to the current raw ugly DOT dump. So forget about the current DOT as it useless compared to the JSON and convert the JSON into DOT so to SVG and HTML?

In D61677#1497130, @NoQ wrote:

• Completely screw current program point and program state dumps and transform them into JSON dumps entirely while keeping it relatively human-readable when it comes to whitespace. This isn't much less readable than the existing dumps but it's also fully machine-readable, and we don't have to maintain two separate dumping facilities.

Would you introduce printJSON() instead of printDiff() in every graph-printer method then just dumping it out into a file? I do not see any generic way.

Thanks for the ideas! I am looking forward for that JSON formatting as it sounds good.

This patch is the sample for every other printDiff() functions, so let us brainstorming. I think it is useless to write out if there is no new information, also it comes with an ugly code. What do you guys think?

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

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

• Sub-state indexing.

I think the entire LocationContext stuff is a huge design issue, and you used its functionality without any hack. If you would rename the getStackFrame to getNextStackFrame or something, it is clear it is not a getter and traversing every frame until the top-frame.

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

Thanks for the review!

index → index+1

When I see ->* I go mad, but we should handle unknown stuff, cool patch.

Great patch! There is only a design problem:
You have negated every isSmall() condition looks like you could write isLarge() instead but there is no connection between these functions.
Could you rename or redesign them? The following would be cool: isLarge() iff !isSmalll and isSmall() iff !isLarge().

The indexing has a problem: zero index would be possible and usually you would match the gray PathDiagnostic bubbles with these notes.

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.

Feature request test is in D61060.

• add-pop-up-notes=true
• Revert clang-format.

@Szelethus, I will revert it.

Thanks you!

I think this functionality is unused because you would split the file into six to reduce the overhead/scroll and that is it.

Thanks you @NoQ for the great idea!

Treat the reports as it is.

I am okay with the grammatical sense, just we are programmers.
Address that only "variable" which is an enum in D60671.

This error found in D59155.

• Rebased

• Rebased

• Rebased

Address comments:

• Removed member variable.
• Introducing PathDiagnosticPopUpPiece(D60670)

It is very good to try one improvement in another similar function.

Nice catch!

In D59121#1448386, @NoQ wrote:

Mmm, that *is* an Assuming... piece, i.e., this is the same code, just the structure of macros is more complicated than usual.

In D59121#1448367, @NoQ wrote:

On second thought, dunno. In the scan-build macro preview it wouldn't show you UINT32_MAX anyway. Maybe let's keep this behavior.

Cleaned up the patch a little bit.

Nice solution.

I like the idea to put connecting stuff together in the same file to create more understandable code. LGTM.

There is no connection with my reverse-engineering reverted patch: https://reviews.llvm.org/D57410?id=184992 ? It evaluates the left and the right side and may it could help.

Cool! I have found this message-semantic idea useful in Unity where every GameObject could talk with each other in a very generic way (https://docs.unity3d.com/ScriptReference/GameObject.SendMessage.html).
I am looking forward for more!

In D59155#1442339, @NoQ wrote:

It seems to me that the original intent here was to only apply quotes to proper nouns (i.e., Assuming this "x" thing of yours, as you call it, is greater than 152), while keeping the common nouns, such as 152, unquoted.

In D57896#1435245, @lattner wrote:

FWIW, my suggestion is *not* to expand names like DRE to decl_ref_expr, I agree that doesn't add clarity to the code. Two possibilities: "dre", or "decl" which is what I would write today.

In D57896#1406812, @lattner wrote:

I can understand Zach's position here, but LLDB has historically never conformed to the general LLVM naming or other conventions due to its heritage. It should not be taken as precedent that the rest of the project should follow.

In any case, I think that it is best for this discussion to take place on the llvm-dev list where it is likely to get the most visibility. Would you mind moving comments and discussions there?