nnethercote / dhat-rs

Heap profiling and ad hoc profiling for Rust programs.
Apache License 2.0
744 stars 36 forks source link

Make `Dhat` constructor private #6

Closed jyn514 closed 3 years ago

jyn514 commented 3 years ago

This avoids typos where a Dhat struct is constructed but doesn't actually do anything.

I have a doctest for this locally, but I don't think it adds much to the documentation. Let me know if I should add it.

diff --git a/src/lib.rs b/src/lib.rs
index 59408d0..c01413f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -193,8 +193,12 @@ use thousands::Separable;
 /// When the first value of this type is dropped, profiling data is written to
 /// file. Only one value of this type should be created; if subsequent values
 /// of this type are created they will have no effect.
+///
+/// ```compile_fail,E0603
+/// let dhat = dhat::Dhat;
+/// ```
jyn514 commented 3 years ago

(this is a breaking change)

nnethercote commented 3 years ago

A couple of questions, because I am unfamiliar with this:

jyn514 commented 3 years ago

I can see it's a breaking change in that there is previously valid code that would no longer work. But that code would be mistaken/useless, so in practice it's unlikely to affect anyone. Right?

Correct, anyone using this already had broken code that wasn't doing anything.

Does this really make the constructor private? Or would it be more accurate to say that it makes the arg-less constructor unusable? (This might be nit-picking, but I'm just trying to understand the change.)

Pedantically, this removes the public 0-argument constructor and adds a private 1-argument constructor. The only argument is () so it has exactly the same information before (the two are isomorphic if that's a familiar term), but rust doesn't allow making zero-argument constructors private.

The doctest, as written, would look strange in the rendered docs, right?

I think so. Here's a screenshot: image

nnethercote commented 3 years ago

I ended up having to add an argument to Dhat in #5, so this is no longer needed.