Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

The following is a series of miscellaneous cleanups.
Attachment #797396 - Flags: review?(jdemooij)
Attachment #797397 - Flags: review?(jwalden+bmo)
Attachment #797398 - Flags: review?(bhackett1024)
Attachment #797400 - Flags: review?(bhackett1024)
Attachment #797401 - Flags: review?(bhackett1024)
Attachment #797397 - Flags: review?(jwalden+bmo) → review+
Attachment #797396 - Flags: review?(jdemooij) → review+
> anonymous-namespaces.patch

What's the benefit of this?
(In reply to Nicholas Nethercote [:njn] from comment #7)
> > anonymous-namespaces.patch
> 
> What's the benefit of this?

Just general code cleanliness. The main measurable effect of this patch is it reduces the number of global symbols among the .o files of the js engine by 3.8% (the number of global symbols in libmozjs-* is unchanged, due to the use of symbol visibilities). It's not important, so I don't mind if such patches aren't accepted.
(In reply to Dan Gohman [:sunfish] from comment #8)
> (In reply to Nicholas Nethercote [:njn] from comment #7)
> > > anonymous-namespaces.patch
> > 
> > What's the benefit of this?
> 
> Just general code cleanliness. The main measurable effect of this patch is
> it reduces the number of global symbols among the .o files of the js engine
> by 3.8% (the number of global symbols in libmozjs-* is unchanged, due to the
> use of symbol visibilities). It's not important, so I don't mind if such
> patches aren't accepted.

I think it would be good to use an empty macro as the name of the anonymous namespaces, such as people who complained about the ability to debug these functions can still set the macro to get a name in their debuggers.
Interesting idea.  But (assuming I understand it) wouldn't it founder when code outside the "anonymous" namespace needs to call a method defined inside it?

namespace ANONYMOUS {

static bool
InitFoo()
{
  return true;
}

}

namespace other_thing {

bool
Exported()
{
  return InitFoo(); // doesn't work if ANONYMOUS expands to anything
}

}

I'm not sure how common this is, but there's some Mozilla code fitting this pattern now.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> bool
> Exported()
> {
>   return InitFoo(); // doesn't work if ANONYMOUS expands to anything
> }

#define USING_NS_ANONYMOUS using namespace anonymous
> #define USING_NS_ANONYMOUS using namespace anonymous

Please, please, no.
Another option would be:

#define ANONYMOUS            \
  anonymous {}               \
  using namespace anonymous; \
  namespace anonymous

in which case one can transparently use the named-anonymous namespace as if it was a real anonymous namespace.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> namespace ANONYMOUS {
> 
> static bool
> InitFoo()
> {
>   return true;
> }
> 
> }
Depends on: 913124
Comment on attachment 797398 [details] [diff] [review]
ra-cleanup.patch

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

Sorry for the delay.
Attachment #797398 - Flags: review?(bhackett1024) → review+
Attachment #797400 - Flags: review?(bhackett1024) → review+
Attachment #797401 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/f2d3df7c6056
https://hg.mozilla.org/mozilla-central/rev/81be63a2e9b1
https://hg.mozilla.org/mozilla-central/rev/f33f92d540df
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.