Wiki Page on introducing how to use this tool: https://wiki.freebsd.org/ShengYiHong/ABIStability
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 61332 Build 58216: arc lint + arc unit
Event Timeline
Comment Actions
The elf parse, ctf parser is all from ctfdump and refactor it into C++. However, because it is cddl licensed, I am wondering if there is any legal concern?
| usr.bin/ctfdiff/ctfdiff.1 | ||
|---|---|---|
| 8 | Should the copyright header be yours? | |
Comment Actions
So not only included the CDDL headers but also some code derived from CDDL files?
cc @imp
Comment Actions
Oh! I'm sorry, but this got lost in my inbox... I've been super busy...
So, it's hard to say. At the very least ctftype.cc needs some kind of acknowledgement that the code was derived from the original CDDL code.
The wrapping in C++ coupled with refactoring suggests that it may have reached the threshold to be a new, transformative work. However, since we're
linking with other CDDL code, I'd err on the side of considering it merely a derived work. This would mean labeling with CDDL and possibly moving
this utility to the cddl subtree.
The files generally need a copyright statement, so I'd start with where you got the elf parser from for those files that contain it (ctftypes.cc and ctfdata.cc
if my eye is right). I'd then add your copyright to that, and add the copyright + license statement to each of the other files. I suspect they should be CDDL
for simplicity, but BSDL 2-Clause is also a viable option.
Comment Actions
I left a couple of comments about a few things that stood out in the first pass, I will likely have more as I dive into the internals.
I do have a feature-related question - would it be possible to add a way of diffing individual types/functions/objects?
e.g. something along the lines of:
$ ctfdiff -t <type_name> file1 file2It would make diagnosing and fixing the breakages easier IMO.
Another thing I've noticed is that you've added support for CTFv2. I would personally be in favor of removing CTFv2 support to simplify the implementation since both ctfconvert and ctfmerge emit CTFv3 (and I'm not aware any in-tree users of CTFv2), but I won't insist.
| usr.bin/ctfdiff/ctfdata.cc | ||
|---|---|---|
| 44 | would it be possible to switch to a string_view here? | |
| 80 | This also applies to other uses of std::cout in this function. | |
| 123 | This also applies to other uses of std::cout in this function. | |
| 153–159 | ||
| 260 | Please #define this number. | |
| 261 | This also applies to other uses of std::cout in this function | |
| 710–713 | ||
| usr.bin/ctfdiff/ctfdiff.cc | ||
| 24–28 | No need to wrap this in a function since it's called only once. | |
| 33 | The nullptr checks below can be replaced std::string_view::empty(). | |
| usr.bin/ctfdiff/ctftype.cc | ||
| 258–259 | ||
| usr.bin/ctfdiff/metadata.cc | ||
| 100 | ||
| 109 | ||
| usr.bin/ctfdiff/ctfdiff.1 | ||
|---|---|---|
| 24 | You can use the SPDX short form as shown in style(9) | |