Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upSimpler alternative dbg!() macro #2361
Conversation
Centril
added
the
T-libs
label
Mar 13, 2018
This comment has been minimized.
This comment has been minimized.
|
Things I'd like to see included from RFC 2173:
EDIT: Regarding point 3. I am now satisfied that we can backwards-compatibly add support for |
This comment has been minimized.
This comment has been minimized.
xftroxgpx
commented
Mar 13, 2018
does it? how come? that seems like a bad idea™ , unless that's also true for (e)println!(), which seems to not be the case(due to magic) ? |
This comment has been minimized.
This comment has been minimized.
Yes, it does. It is not a bad idea, but instead good design. Not having move semantics would mean that you would have to evaluate the expression passed twice, and thus also any side effects. If you don't want to move |
This comment has been minimized.
This comment has been minimized.
xftroxgpx
commented
Mar 13, 2018
|
Ah, my bad: I forgot (deleted)what I wanted to write before realizing that
I guess I don't see what is gained by moving it? And why doesn't it happen for https://play.rust-lang.org/?gist=178efc0e3b3c9420dde9df730c15d3cf&version=stable |
This comment has been minimized.
This comment has been minimized.
repax
commented
Mar 13, 2018
|
I think this macro should be conditionally compiled, just like debug assertions are. It's rather important to distinguish debugging info from other kinds of logging. If |
This comment has been minimized.
This comment has been minimized.
|
@Centril I’ve added some wording for the exact formatting and for ownership. This RFC makes no plan for specialization. I don’t see the point of printing a line that says nothing about the value instead of failing to compile with @repax I disagree, for reasons given in the Alternative section of the RFC. |
This comment has been minimized.
This comment has been minimized.
repax
commented
Mar 13, 2018
|
@SimonSapin, debug logging for bugs that only manifest during optimisation is a fringe case. Designing the Following the principle of least astonishment is not a bad decision. In this case, debug assertions and debug messages are both assumed to belong to debugging. If you write a routine that include We could not permit |
This comment has been minimized.
This comment has been minimized.
|
I’ve always found the phrase “principle of least astonishment” very unconvincing. It sounds fancy, but it does nothing to justify what is found to be “astonishing”, by whom, or why. I my mind |
This comment has been minimized.
This comment has been minimized.
repax
commented
Mar 13, 2018
|
It's easy to make mistakes -- that's all. You can, and should, leave debug assertions when committing, but not debug messages? You still haven't motivated why the fringe case of optimisations-only bugs have precedence to regularity in the design. One aim of good design should be to minimise the risk of errors, not just push the responsibility to someone else, or brush off good heuristics as fancy sounding. At least, that's something I believe. Perhaps this macro is better suited in a library, or with a different name? |
This comment has been minimized.
This comment has been minimized.
|
And you haven’t argued how disabling printing in release mode is more "regular" (or what that means). As to having this macro in a library, that makes it effectively worthless in my opinion. As already discussed in #2173, the whole point is having something that easier to use than |
This comment has been minimized.
This comment has been minimized.
nielsle
commented
Mar 14, 2018
|
Related idea. How about changing the syntax for 'debug!', so that the following three statements have the same effect. debug!("Values x = {:?} y = {:?}", x, y);
debug!("Values x = {:?}", x; y);
debug!("Values"; x, y);Note arguments after the semicolon are formatted differently. |
This comment has been minimized.
This comment has been minimized.
Entirely orthogonal if you are talking about the |
This comment has been minimized.
This comment has been minimized.
The point is quite important and is discussed at:
The value of specialization and fallback here is that you can start debugging anywhere in generic code without having a |
This comment has been minimized.
This comment has been minimized.
rpjohnst
commented
Mar 14, 2018
|
Bikeshed: name it |
This comment has been minimized.
This comment has been minimized.
|
@rpjohnst The bikeshed so far: https://github.com/Centril/rfcs/blob/rfc/quick-debug-macro/text/0000-quick-debug-macro.md#bikeshed-the-name-of-the-macro As a Haskeller I'm almost compelled to favor a Haskell based name, but.. I think this will cause users to think that the stack trace will somehow be printed out if you call |
Centril
referenced this pull request
Mar 16, 2018
Closed
add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071
This comment has been minimized.
This comment has been minimized.
|
I just want to register my support for this proposal; I think it strikes a good balance between packaging up the most impactful bits of the original RFC into a much more streamlined macro. This feels like a good foundation that we can gradually extend as time goes on. |
This comment has been minimized.
This comment has been minimized.
|
Regarding specialization and |
This comment has been minimized.
This comment has been minimized.
|
I like this proposal. I'd like to voice my support for automatically disabling these in release mode. It wouldn't be that helpful for investigation of heisenbugs that happen only in release mode, since mere presence of this statement changes generated code and timing (due to locks on stdout). On many occasions I have had forgotten to remove my debug |
This comment has been minimized.
This comment has been minimized.
|
Idea: |
This comment has been minimized.
This comment has been minimized.
|
@clarcharr That definition of @rust-lang/libs It seems the only outstanding issue remaining is whether to disable outputs on Pros of only keeping output when
Cons:
|
This comment has been minimized.
This comment has been minimized.
ssokolow
commented
Apr 2, 2018
•
|
Intuition also suggests that Conversely, if the user encounters |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
ssokolow
commented
Apr 2, 2018
•
Good point.
Even better, as I see it. Being able to enable the assertions and prints separately could be useful.
That was intended to be the thrust of my point but it didn't occur to me to be clear about that. It seems I'm still not fully recovered from that all-nighter I pulled a couple of days ago. |
This comment has been minimized.
This comment has been minimized.
I think that the RFC as written is forward compatible with that; for initial simplicity I'd like to not that go route for now; but we can extend the macro in the future if we so wish. |
This comment has been minimized.
This comment has been minimized.
There is some precedent that the |
Centril
merged commit 153188b
into
rust-lang:master
Sep 17, 2018
This comment has been minimized.
This comment has been minimized.
|
Huzzah! This RFC has been merged! Tracking issue: rust-lang/rust#54306 Took a bit more time than I wanted to get to this; but I am working on an implementation right now. |
This comment has been minimized.
This comment has been minimized.
swfsql
commented
Sep 20, 2018
|
I use a small similar code I called "log_here". It has macros that functions as "newtypes" for format/error/warn/info/etc, that adds the [file:line] info as prefix as well. So the usage is format_here/error_here/warn_here/and so on.. |
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Sep 20, 2018
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Sep 21, 2018
This comment has been minimized.
This comment has been minimized.
gilescope
commented
Sep 23, 2018
|
Many a time I add println!("{:#?}", x) in the middle of the code, only to be told by the compiler that I need to borrow x as I am using it further down. I haven't read a good argument for why we move rather than borrow by default? I would have thought borrowing x would be more ergonomic (read less annoying)... (sorry - I'm coming to the party late here) |
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Sep 23, 2018
This comment has been minimized.
This comment has been minimized.
|
@gilescope |
This comment has been minimized.
This comment has been minimized.
gilescope
commented
Sep 23, 2018
|
Hmm, will need to check that, but the RFC seemed to be suggesting that it was going to move not borrow x for dbg!(x): Move semantics The dbg!(x) macro moves the value x and takes ownership of it, unless the type of x implements Copy, and returns x unchanged. If you want to retain ownership of the value, you can instead borrow x with dbg!(&x). |
This comment has been minimized.
This comment has been minimized.
|
The macro takes ownership of the input passed because it then returns it, so you can do |
This comment has been minimized.
This comment has been minimized.
|
Hmm... come to think of it, that seems like it may have the unfortunate side effect of being used like this: fn foo(arg: Bar) {
dbg!(arg);
baz(&arg); // error: `arg` was moved above!
}Perhaps we need some sort of unused result lint? |
This comment has been minimized.
This comment has been minimized.
|
@mark-i-m maybe we can use (you can write PS: let's move the discussion to the tracking issue :) rust-lang/rust#54306 |
This comment has been minimized.
This comment has been minimized.
|
I think this should not be If moving / requiring a manual |
This comment has been minimized.
This comment has been minimized.
|
OK; I agree with you on
but I don't agree with this. This was an important design choice in allowing the user to write |
This comment has been minimized.
This comment has been minimized.
|
I completely agree with @Centril here. The ability to use this in the middle of an expression is the key feature IMHO. |
This comment has been minimized.
This comment has been minimized.
|
I don't think it will be necessary anyways to do such a change because the macro already tells the user what the problem is clearly: 19 | baz(&arg);
| ^^^ value used here after moveThe main problem is that the error is "incorrectly spanned": 4 | tmp => {
| --- value moved hereYou would rather want to say: L | dbg!(arg);
| --- value moved here; try borrowing with `&arg` instead?This seems like a general problem with macros which we should find a general solution to rather than institute an ergonomics regression... |
bors
added a commit
to rust-lang/rust
that referenced
this pull request
Sep 25, 2018
This comment has been minimized.
This comment has been minimized.
|
@gilescope Ah, my bad, I thought you meant |
This comment has been minimized.
This comment has been minimized.
lachlansneff
commented
Sep 28, 2018
|
Doesn't this evaluate expr twice? |
This comment has been minimized.
This comment has been minimized.
|
If you use it in an expression, I would imagine it just evaluates it once, right? foo(dbg!(<fmt>, <expr>));I imagine it would expand to something like foo({
let value = <expr>;
println!(<fmt>, value);
value
}); |
This comment has been minimized.
This comment has been minimized.
|
The macro is defined as: macro_rules! dbg {
($val:expr) => {
match $val {
tmp => {
eprintln!("[{}:{}] {} = {:#?}",
file!(), line!(), stringify!($val), &tmp);
tmp
}
}
}
}which means that |
This comment has been minimized.
This comment has been minimized.
lachlansneff
commented
Sep 28, 2018
|
Whoops, my mistake. Thanks! |
SimonSapin commentedMar 13, 2018
•
edited by Centril
This is a simpler and more opinionated counter-proposal to RFC 2173.
Rendered
Tracking issue