Add demangle(1) command
Review Request #2518 — Created Feb. 15, 2020 and submitted
Information | |
---|---|
jbk | |
illumos-gate | |
12310 | |
Reviewers | |
general | |
Add demangle(1) command
Run with various inputs both cmdline and via stdin and examined the output.
- 3
- 0
- 20
- 1
- 24
Description | From | Last Updated |
---|---|---|
So if we fail for some other reason, should we still output what the original symbol was for consistency in ... |
|
|
Should we warn at all to stderr about failures to demangle a name? |
|
|
I don't think launching into a discussion of what a mangled symbol is right at the start is necessarily the ... |
|
-
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) We often distinguish between a fatal error and an error for usage. Presumably we should here too?
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Rather than cast zero to a pointer type, why not just use NULL, and then you shouldn't need the pointer either. Also, this returns a pointer type, see head/xlocale.h, so don't do a comparison with zero.
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Please give the user a more actionable error message. If I saw an error message that was something like: 'demangle: newlocale: no such file or directory' I would have no idea what to do. At least something like, 'failed to construct the C locale: <error string>' would be a bit more useful and would speed up the debugging process/confusion.
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Should we use warnx here for consistency of having the program name at the start?
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Presumably i should match the type of argc, which is an int?
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Should we warn at all to stderr about failures to demangle a name?
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) It's probably worth a comment or explanation of how this heuristic was determined. Without context, it seems arbitrary and it's not clear why certain characters are there and not others.
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Please give the user a better error message here and all the other uses of err in this function. I'm only commenting on it here.
-
usr/src/cmd/demangle/demangle.c (Diff revision 2) Shouldn't we be just checking that fprintf returned -1? Also, since you aren't flushing here and are relying on something else to the -1, I'm not sure why you'd check ferror, given that most of the time it probably will be buffered and if not, then the fprintf should propagate the error, no?
-
-
usr/src/man/man1/demangle.1 (Diff revision 2) I don't think launching into a discussion of what a mangled symbol is right at the start is necessarily the best way to get going. I probably would start with a summary of what the program actually does, like you have at line 46-49. I think that's a bit more user friendly, otherwise what this does is buried behind an explanation of symbol mangling. I'm assuming you did it this way because that's how the GNU c++filt manual is written.
-
usr/src/man/man1/demangle.1 (Diff revision 2) You should refer to the actual operand in this description to refer back to what's in the synopsis.
-
usr/src/man/man1/demangle.1 (Diff revision 2) Most manuals spell out stdin to be 'standard in' rather than use the the C symbol name.
-
-
-
usr/src/man/man1/demangle.1 (Diff revision 2) I don't think it quite actually causes us to only demangle symbols from the language. Instead, it always tries to interpret everything as being from the language. If we had names that are valid in both, then we would interpret it in the specified one, which doesn't quite match what that sentence is saying to me.
-
usr/src/man/man1/demangle.1 (Diff revision 2) The xxxx for this looks a little weird at first glance as it makes it seem like it was a fill me in and not trying to be a four character value.
-
usr/src/man/man1/demangle.1 (Diff revision 2) I would give a brief explanation of these. For example, for rust I would describe which of its mangling formats are supported and I would explain that auto attempts to figure out what i tis automatically.
-
usr/src/man/man1/demangle.1 (Diff revision 2) We normally don't put examples in these kinds of lists. Take a look at sleep(1) for an example.
-
usr/src/man/man1/demangle.1 (Diff revision 2) With each example you should have a textual description of what it is demonstrating.
-
usr/src/man/man1/demangle.1 (Diff revision 2) Should readers be referred to any other utilities in a SEE ALSO or pages? We should probably explicitly call out the stability of this, in particular that the options are probably not stable and the output format isn't committed.
-
Sorry for the delay in getting back to this. I saw that there were a number of unresolved things related to non-questions which is why I hadn't looked at the new version. It wasn't clear to me if you had wanted additional feedback or not. I've left follow up on the original feedback and a few more additional things I've noticed below.
-
usr/src/cmd/demangle/demangle.c (Diff revisions 1 - 3) So if we fail for some other reason, should we still output what the original symbol was for consistency in addition to the warning or is that not necessary?
-
usr/src/man/man1/demangle.1 (Diff revisions 1 - 3) I think you probably want to phrase this more concretely as: .Sh INTERFACE STABILITY The command line options are .Sy Uncommitted . The output format is .Sy Not-an-Interface . We should use the standard names here from attributes(5).
-
usr/src/cmd/demangle/demangle.c (Diff revision 3) Given that you've changed how the symbol version works and warns, should the custr version match?
-
Thanks for working through this stuff. Seems reasonable.
-
usr/src/man/man1/demangle.1 (Diff revision 5) Is it worth mentioning here that symbol boundaries are only determined by the C locale or is that too much?