components/nacl/loader/nacl_listener.cc - Issue 315323002: Disable Warning 4702 for the PGO builds.

Side by Side Diff: components/nacl/loader/nacl_listener.cc

Issue 315323002: Disable Warning 4702 for the PGO builds. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Update the comment. Created 6 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
1 // Copyright 2013 The Chromium Authors. All rights reserved.	1 // Copyright 2013 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "components/nacl/loader/nacl_listener.h"	5 #include "components/nacl/loader/nacl_listener.h"

6	6

7 #include <errno.h>	7 #include <errno.h>

8 #include <stdlib.h>	8 #include <stdlib.h>

9	9

10 #if defined(OS_POSIX)	10 #if defined(OS_POSIX)

(...skipping 457 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
468 #if defined(OS_WIN)	468 #if defined(OS_WIN)

469 args->broker_duplicate_handle_func = BrokerDuplicateHandle;	469 args->broker_duplicate_handle_func = BrokerDuplicateHandle;

470 args->attach_debug_exception_handler_func = AttachDebugExceptionHandler;	470 args->attach_debug_exception_handler_func = AttachDebugExceptionHandler;

471 args->debug_stub_server_port_selected_handler_func =	471 args->debug_stub_server_port_selected_handler_func =

472 DebugStubPortSelectedHandler;	472 DebugStubPortSelectedHandler;

473 #endif	473 #endif

474 #if defined(OS_LINUX)	474 #if defined(OS_LINUX)

475 args->prereserved_sandbox_size = prereserved_sandbox_size_;	475 args->prereserved_sandbox_size = prereserved_sandbox_size_;

476 #endif	476 #endif

477	477

478 NaClChromeMainStartApp(nap, args);	478 NaClChromeMainStartApp(nap, args);
	bradn 2014/06/10 16:42:47 I'm a little uncomfortable with this being removed I'm a little uncomfortable with this being removed. Could we not introduce a define and do this conditionally? Sébastien Marchand 2014/06/10 17:03:33 I could do this but the NaCLChromeMainStartApp fun Show quoted text On 2014/06/10 16:42:47, bradn wrote: > I'm a little uncomfortable with this being removed. > Could we not introduce a define and do this conditionally? I could do this but the NaCLChromeMainStartApp function doesn't return and this cause an 'unreachable code' warning to be triggered... bradn 2014/06/10 17:35:31 So we have this here for a very particular reason. Show quoted text On 2014/06/10 17:03:33, Sébastien Marchand wrote: > On 2014/06/10 16:42:47, bradn wrote: > > I'm a little uncomfortable with this being removed. > > Could we not introduce a define and do this conditionally? > > I could do this but the NaCLChromeMainStartApp function doesn't return and this > cause an 'unreachable code' warning to be triggered... So we have this here for a very particular reason. This is very much an assert. We're setting up nacl at this point, potentially doing non-intuitive things and want to be certain that control flow does not get beyond here. As you're also disabling the unreachable code warning in this change, why is this any issue anyhow? Sébastien Marchand 2014/06/10 17:40:49 I've address Peter's comment her https://code.goog Show quoted text On 2014/06/10 17:35:31, bradn wrote: > On 2014/06/10 17:03:33, Sébastien Marchand wrote: > > On 2014/06/10 16:42:47, bradn wrote: > > > I'm a little uncomfortable with this being removed. > > > Could we not introduce a define and do this conditionally? > > > > I could do this but the NaCLChromeMainStartApp function doesn't return and > this > > cause an 'unreachable code' warning to be triggered... > > So we have this here for a very particular reason. > This is very much an assert. We're setting up nacl at this point, potentially > doing non-intuitive things and want to be certain that control flow does not get > beyond here. > > As you're also disabling the unreachable code warning in this change, why is > this any issue anyhow? I've address Peter's comment her https://code.google.com/p/chromium/issues/detail?id=380175#c29 ("We should kill this particular NOTREACHED() regardless -- it's legitimately unreachable and shouldn't be in the codebase"), but if you feel that there's a strong reason for it to be there then I could restore it... It's just that it might potentially break the official build one day (as the 'unreachable code' warning isn't turned off on it, so the compiler might realize that this code can't be reached...)
479 NOTREACHED();

480 }	479 }

OLD	NEW

« build/common.gypi ('K') | « build/common.gypi ('k') | no next file » | no next file with comments »