On 07/21/2017 06:23 AM, Jakob Kummerow wrote:
Hi Mostyn,
this is awesome, I'm very excited about it! Thank you for working on this.
A few comments:
I see you've added a few #pragmas around existing "using namespace v8"
directives. Those should just be removed entirely, they're a style guide
violation anyway. That burden should not be on this CL though, so I'm fine
with the pragmas as a temporary workaround.
OK, I will leave TODO notes where applicable.
I'm not a fan of the "namespace ANON" solution. I think we should just
rename stuff as necessary to avoid name clashes. In that case we could even
keep the anonymous namespaces (to make the linker's life easier by hiding
those symbols from it), right? Or are they a problem for jumbo builds in
general?
I have encountered two types of errors due to anonymous namespaces. The
first are name clashes, which are simple enough to deal with.
The second is when a class that is outside the anonymous namespace has a
member whose type is inside the anonymous namespace. GCC gives ODR
related warnings in this case, because you might (even though we don't,
AFAIK) use the class in another translation unit which can't see the
type from the anonymous namespace eg:
In file included from gen/base/trace_event/common/v8_base_jumbo_5.cc:58:0:
gen/base/trace_event/common/../../../../../../v8/src/wasm/wasm-interpreter.cc:2417:7:
error: ‘v8::internal::wasm::WasmInterpreterInternals’ has a field
‘v8::internal::wasm::WasmInterpreterInternals::codemap_’ whose type uses
the anonymous namespace [-Werror]
class WasmInterpreterInternals : public ZoneObject {
In this instance, WasmInterpreterInternals is outside the anonymous
namespace, and the type of codemap_ is inside.
GCC suppresses this warning in the main input file, but jumbo builds
break the workaround (since the main input file includes the real source
files). Some background:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=29365
Anonymous namespaces prevent symbols from being used in other
translation units (and avoid clashes in the process). But we already
build with hidden symbol visibility by default (-fvisibility=hidden on
gcc/clang, and some corresponding solution for windows) and export only
the required symbols.
So we could either move more code into the anonymous namespace, or move
more code out of the anonymous namespace. I don't think the anonymous
namespace adds much benefit and is often misunderstood, so I would
prefer the latter, and maybe even to drop the anonymous namespace where
it causes trouble.
Instead of the sequence:
#undef foo
#define foo bar
// use foo here
I would prefer if we maintained good hygiene and consistently did:
#define foo
// use foo here
#undef foo
If it's too difficult to find all places where we forget to "#undef __",
let me help you find them!
Done for the macros in question.
IMO it's a little easier to keep the "undef then define" style working,
since it's more obvious when copy+pasting existing code, but it's not
that important. Jumbo builds will break in obvious ways if we forget to
#undef at the bottom of a file.
What are the reasons why you have to skip a few files
via jumbo_excluded_sources? I.e. what would it take to get them
un-blacklisted?
The generated source files are (I think) produced from the same
template, giving symbol clashes if they're included in the same
translation unit. This probably requires introducing a specified
namespace to the generator script. I would like to defer this to a
later CL.
The regular source files either had complicated issues that I don't
fully understand yet, or they created lots of issues and I wanted to get
something working quickly first. I might be able to fix some of these
before my first CL is ready to land, but it would be easier to defer
some of this work.
And one final nit: Thanks for finding and adding all those missing include
guards! V8's header include guards begin with "V8_", e.g. V8_FILENAME_H.
This is to prevent name clashes with Chromium ;-)
Done.
-Mostyn.
--
Mostyn Bramley-Moore
Opera TV
[email protected]
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
