1082128 - Encapsulate id to string conversion in DMD

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Andrew McCreight [:mccr8]]

Right now, the conversion of ids to strings is spread out among a couple of functions and a number of variables.  This makes it harder to use in other functions.  We should just wrap all the state into a stack class.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Make id to string conversion in DMD use a class.

Nick, how does this look to you, generally speaking.  Suggestions welcome on naming.

Comment 2

[Andrew McCreight [:mccr8]]

ToString() isn't used yet, but I need it in my conservative heap scanning patch, so I figured I'd just add it here.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8508361 [details] [diff] [review]
Make id to string conversion in DMD use a class.

Review of attachment 8508361 [details] [diff] [review]:
-----------------------------------------------------------------

Nice encapsulation.

::: memory/replace/dmd/DMD.cpp
@@ +1594,5 @@
>  {
>    return gIsDMDRunning;
>  }
>  
> +class StringConverter MOZ_FINAL

I'd call it ToIdStringConverter.

@@ +1608,5 @@
> +  // it's been seen before.
> +  const char* ToIdString(const void* aPtr);
> +
> +  // Converts a pointer to its literal string representation.
> +  const char* ToString(const void* aPtr);

This doesn't belong in this class.

@@ +1641,5 @@
>  //   inspecting/debugging the JSON output, non-numeric indices are easier to
>  //   search for than numeric indices.
>  //
> +char*
> +StringConverter::Base32(uint32_t aN)

It's more version control churn, but I'd move Base32() and ToIdString() inside the class declaration.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Nice encapsulation.
Thanks.  With ToString moved out of the class, it isn't really needed, but I think this is an improvement anyways.

> I'd call it ToIdStringConverter.
Sounds good.

> This doesn't belong in this class.
Yeah, I guess a tiny char buffer is not enough shared state for it to make any sense. ;)  I can make ToString() some kind of static method.

> It's more version control churn, but I'd move Base32() and ToIdString()
> inside the class declaration.

I disagree. I think they are reasonably long, fairly complicated methods. Why do you want them moved inside?

Comment 5

[Nicholas Nethercote [inactive]]

> > It's more version control churn, but I'd move Base32() and ToIdString()
> > inside the class declaration.
> 
> I disagree. I think they are reasonably long, fairly complicated methods.
> Why do you want them moved inside?

We have different ideas about what constitutes a long, fairly complicated method :)

My default inclination is to put definitions inside the class because (a) it avoids duplicating the signature, and (b) you don't have to decide where to put comments about the function -- in your patch you are inconsistent about this.

I would go against this default inclination when the class declaration is in a header and putting the definition makes more sense in a .cpp file, or when the method is *really* long, e.g. 100s of lines.

But it's really not that big a deal, so I'm fine if you want to leave it as you have it.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Make id to string conversion in DMD use a class.

I inlined the methods because I suppose that in a cpp file it doesn't really matter one way.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6437da855571

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8511155 [details] [diff] [review]
Make id to string conversion in DMD use a class.

Review of attachment 8511155 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 8

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37e6b0283f65

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37e6b0283f65