-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DONOT Merge Add formatted printing of pointers #1285
DONOT Merge Add formatted printing of pointers #1285
Conversation
This has debug logging (wink_log) which will be removed before this is merged. |
std/fmt/index.zig
Outdated
/// Renders fmt string with args, calling output with slices of bytes. | ||
/// If `output` returns an error, the error is returned from `format` and | ||
/// `output` is not called again. | ||
pub fn format(context: var, comptime Errors: type, output: fn (@typeOf(context), []const u8) Errors!void, comptime fmt: []const u8, args: ...) Errors!void { | ||
//wink_log_strln("format:+ ", fmt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use std.debug.warn("format:+ ", fmt);
or even @compileLog("format:+", fmt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with using warn is that it uses std.format so its recursive and generates extra output, which is why I used "wink_log" which outputs the directly to stdout.
var value = "abc"; | ||
try testFmt("var []u8: abc\n", "var []u8: {}\n", value); | ||
} | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use @intToPtr
to test some values here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, I added these just because there weren't tests of quoted strings so I thought I'd add a couple. Could you provide a more information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're only checking that the prefix is correct, because pointers have non-deterministic values at runtime. But you could use @intToPtr(*T, 0xdeadbeef)
and then you can look for "T@0xdeadbeef"
using testFmt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, another option is to use @ptr2Int using "real" addresses:
{
var buf1: [32]u8 = undefined;
var buf2: [32]u8 = undefined;
const value = "abc";
// Print Pointer Path3
const expected = try bufPrint(buf2[0..], "&value: u8@{x}", @ptrToInt(&value));
const actual = try bufPrint(buf1[0..], "&value: {p}", &value);
wink_log_strln("expected=", expected);
wink_log_strln("actual=", actual);
try testExpectedActual(expected, actual);
}
fn testExpectedActual(expected: []const u8, actual: []const u8) !void {
if (mem.eql(u8, expected, actual)) return;
std.debug.warn("\n====== expected this output: =========\n");
std.debug.warn("{}", expected);
std.debug.warn("\n======== instead found this: =========\n");
std.debug.warn("{}", actual);
std.debug.warn("\n======================================\n");
return error.TestFailed;
}
I like using "real" addresses where we can, do you have a preference?
4fad90b
to
bb97863
Compare
Just rebased since it had been a few days. Also, this will kick off a new CI build as the last one failed in appveyro for now apparent reason. |
bb97863
to
63aad0a
Compare
Lastest change, 63aad0a, removes wink_log and is rebased on ToT as of a few minutes ago. |
Needs support for {p} support I added to std/fmt/index.zig: ziglang/zig#1285 Should be in master soon.
I found a bug, you can't print the address of a struct with a custom format routine. Will add a test and try to fix. |
Added 'p' as a new formatting character. Add tests that print pointers of various sizes and covers all the paths. One existing test needed to be change, the Struct test at line 1055, a {} changed to {p}.
63aad0a
to
1b26c81
Compare
The solution I came up with to allow a custom format routine of a structure to have its address printed was to add a pubic formatAddress routine and then a custom format routine can invoke it to print the address. There are certainly other solutions, such as detect the {p} before calling the custom format routine, but it gives the custom format routine more control. This has passed all tests locally. I'll leave the DONOT Merge on this change until you review. |
Needs support for {p} support I added to std/fmt/index.zig: ziglang/zig#1285 Should be in master soon.
@andrewrk, review please. |
I'll have a look at this tomorrow morning and get it working. |
Txs, AFAIK it is working, but I'm sure there are improvements :) One thing is I'd like to see testExpectedActual be public, but not sure where it might go. I'll be glad to make any changes you'd like to see |
I did a simpler implementation, and used |
Added 'p' as a new formatting character. Add tests that
print pointers of various sizes and covers all the paths.
One existing test needed to be change, the Struct test at line 1109,
a {} changed to {p}.