This is an archive of the discontinued LLVM Phabricator instance.

[llvm-jd] introduces a new tool for diffing JSON
Needs ReviewPublic

Authored by cjdb on May 30 2023, 12:58 PM.

Details

Summary

llvm-jd is a C++ port of [jd], which is a JSON diff tool. It supports the subset
that we need for LLVM.

FileCheck is well-suited to output that follows a particular sequence and where
delta outputs are fairly readable upon inspection. Our JSON output can sometimes
appear in different orders, and due to the structuring of JSON, it's often
difficult to work out where in the JSON object something is. llvm-jd is able to
report the exact position of a diff more directly, and can also take order (or
lack thereof) into account.

[llvm-jd] adds UI I/O

[llvm-jd] adds scalar support

[llvm-jd] adds array support

[llvm-jd] adds object support

[llvm-jd] fixes the output so there's no space between commas

[llvm-jd] sets the return code for diffs to 1

[llvm-jd] adds multiset capabilities

jd supports multisets, which allow us to test JSON arrays without regard
for order.

Diff Detail

Event Timeline

cjdb created this revision.May 30 2023, 12:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 12:58 PM
cjdb requested review of this revision.May 30 2023, 12:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2023, 12:58 PM
cjdb added inline comments.May 30 2023, 1:03 PM
llvm/docs/llvm-jd.rst
1–68

Seems I made two readme files. I'll merge them together.

Precommit CI found issues that need to be addressed.

Thank you for working on this!

llvm/docs/llvm-jd.rst
21–22

Building instead of Installation?

60

only for -f=jd?

llvm/include/llvm/Support/JSON.h
1081–1087

Same below.

However, shouldn't we be using the functionality from https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/Hashing.h instead of rolling our own?

llvm/utils/llvm-jd/llvm-jd.cpp
20–21

I think some of this logic will be made easier with a raw_ostream-based interface instead of using the super slow iostream functionality from the STL, WDYT about using that? (Note, we've got raw_fd_ostream which has special logic for handling stdout.)

24

Are we using this include?

62
67–69
86–88

I'll stop commenting on style issues; you should take a pass through to make sure things match our usual style guidelines.

114

I love this kind of comment, thank you for having it!

199

Gross, but clever. :-D I think this could be made a bit more clean with a raw_fd_ostream interface so we can try to open the file and then if that fails, open stdout instead.

jroelofs added inline comments.Jun 7 2023, 1:08 PM
llvm/lib/Support/JSON.cpp
931

This newline should stay. </UB pedant>

llvm/utils/llvm-jd/llvm-jd.cpp
429

missing trailing newline

cjdb added inline comments.Jun 7 2023, 1:25 PM
llvm/include/llvm/Support/JSON.h
1081–1087

Thanks for putting this on my radar!

llvm/utils/llvm-jd/llvm-jd.cpp
86–88

I was hoping that we could add braces since this is a new tool. Naming, on the other hand, is just negligence and shall be corrected.

429

There is a trailing newline: Phab just doesn't show it. (It'll tell you that there's a missing new line, and I have VS Code set up to ensure that there's always exactly one new line at the end of the file.)

Unless you're saying there should be two?

jroelofs added inline comments.Jun 7 2023, 1:45 PM
llvm/utils/llvm-jd/llvm-jd.cpp
429

nvm, you're right, and looks like that restriction went away in C++11... carry on ;)

aaron.ballman added inline comments.Jun 8 2023, 5:43 AM
llvm/utils/llvm-jd/llvm-jd.cpp
86–88

New code should continue to follow our usual developer policies instead of forging a new path.

tschuett added inline comments.
llvm/utils/llvm-jd/llvm-jd.cpp
167

The `and' s are cute, but no thanks.

187

Unfortunately, std::span is only available with C++20. I would instead go for llvm::ArrayRef.