Sorry for the delay. I checked src/asmjs and src/wasm. Left three comments, LGTM when they are addressed.
On Tue, Aug 1, 2017 at 11:20 PM <[email protected]> wrote: > I have some general lgtm's for this CL, but I'm still looking for OWNERS > to review changes in the following directories: > src/asmjs/ > src/wasm/ > src/inspector/ > src/parsing/ > > Any recommendations for people that aren't currently on vacation? > > > -Mostyn. > > On Friday, July 21, 2017 at 8:21:39 PM UTC+2, Jakob Kummerow wrote: > >> On Fri, Jul 21, 2017 at 3:33 AM, Mostyn Bramley-Moore <[email protected]> >> wrote: >>> >>> 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. >> >> >> OK, I'm fine with dropping the anonymous namespaces (and/or as a first >> step moving code out of them as required). >> >> 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. >> >> >> Agreed that it might be easier, but cleaning up after oneself seems more >> elegant ;-) >> >> >>> 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. >> >> >> OK, sure, makes sense. >> >> Thanks again for doing this work! >> >> -- > -- > 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. > -- Clemens Hammacher Software Engineer [email protected] Google Germany GmbH Erika-Mann-Straße 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person. -- -- 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.
