Encapsulate id to string conversion in DMD

RESOLVED FIXED in mozilla36

Status

()

Core
DMD
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8508361 [details] [diff] [review]
Make id to string conversion in DMD use a class.

Nick, how does this look to you, generally speaking.  Suggestions welcome on naming.
Attachment #8508361 - Flags: feedback?(n.nethercote)
(Assignee)

Comment 2

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

Updated

3 years ago
Blocks: 1058178
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.
Attachment #8508361 - Flags: feedback?(n.nethercote) → feedback+
(Assignee)

Comment 4

3 years ago
(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?
> > 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.
(Assignee)

Updated

3 years ago
Summary: Make id to string conversion in DMD class-ier → Encapsulate id to string conversion in DMD
(Assignee)

Comment 6

3 years ago
Created attachment 8511155 [details] [diff] [review]
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
Attachment #8511155 - Flags: review?(n.nethercote)
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
Attachment #8511155 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/37e6b0283f65
https://hg.mozilla.org/mozilla-central/rev/37e6b0283f65
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Updated

3 years ago
Attachment #8508361 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.