Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(368)

Issue 20843012: Extract hardcoded error strings into a single place and replace them with enum. (Closed)

Created:
7 years, 4 months ago by loislo
Modified:
7 years, 4 months ago
CC:
v8-dev
Visibility:
Public.

Description

Extract hardcoded error strings into a single place and replace them with enum. I'd like to propagate bailout reason to cpu profiler. So I need to save it into heap object SharedFunctionInfo. But: 1) all bailout reason strings spread across all the sources. 2) they are native strings and if I convert them into String then I may have a performance issue. 3) one byte is enough for 184 bailout reasons. Otherwise we need 8 bytes for the pointer. Also I think it would be nice to have error strings collected in one place. In that case we will get additional benefits: It allows us to keep this set of messages under control. It gives us a chance to internationalize them. It slightly reduces the binary footprint. From the other hand the developers have to add new strings into that enum. BUG= R=jkummerow@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=16024

Patch Set 1 #

Patch Set 2 : underscores were removed from constants #

Patch Set 3 : enum was converted into standard v8 macro #

Total comments: 10

Patch Set 4 : styles fixed #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+774 lines, -488 lines) Patch
M src/arm/builtins-arm.cc View 7 chunks +10 lines, -10 lines 0 comments Download
M src/arm/code-stubs-arm.cc View 15 chunks +18 lines, -19 lines 0 comments Download
M src/arm/codegen-arm.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/arm/debug-arm.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M src/arm/full-codegen-arm.cc View 5 chunks +10 lines, -10 lines 0 comments Download
M src/arm/lithium-arm.h View 1 chunk +1 line, -1 line 0 comments Download
M src/arm/lithium-arm.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M src/arm/lithium-codegen-arm.h View 1 chunk +1 line, -1 line 0 comments Download
M src/arm/lithium-codegen-arm.cc View 13 chunks +15 lines, -15 lines 0 comments Download
M src/arm/macro-assembler-arm.h View 1 chunk +3 lines, -3 lines 0 comments Download
M src/arm/macro-assembler-arm.cc View 1 21 chunks +28 lines, -27 lines 0 comments Download
M src/code-stubs-hydrogen.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M src/compiler.h View 2 chunks +3 lines, -3 lines 0 comments Download
M src/compiler.cc View 1 2 3 7 chunks +13 lines, -11 lines 1 comment Download
M src/hydrogen.h View 2 chunks +2 lines, -2 lines 0 comments Download
M src/hydrogen.cc View 53 chunks +63 lines, -68 lines 0 comments Download
M src/ia32/builtins-ia32.cc View 7 chunks +10 lines, -10 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 17 chunks +21 lines, -22 lines 0 comments Download
M src/ia32/codegen-ia32.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/ia32/debug-ia32.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/ia32/deoptimizer-ia32.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/ia32/full-codegen-ia32.cc View 7 chunks +11 lines, -11 lines 0 comments Download
M src/ia32/ic-ia32.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/ia32/lithium-codegen-ia32.h View 1 chunk +1 line, -1 line 0 comments Download
M src/ia32/lithium-codegen-ia32.cc View 7 chunks +7 lines, -7 lines 0 comments Download
M src/ia32/lithium-ia32.h View 1 chunk +1 line, -1 line 0 comments Download
M src/ia32/lithium-ia32.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M src/ia32/macro-assembler-ia32.h View 1 chunk +3 lines, -3 lines 0 comments Download
M src/ia32/macro-assembler-ia32.cc View 1 22 chunks +29 lines, -28 lines 0 comments Download
M src/ia32/stub-cache-ia32.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/lithium.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/mips/builtins-mips.cc View 7 chunks +10 lines, -10 lines 0 comments Download
M src/mips/code-stubs-mips.cc View 20 chunks +23 lines, -24 lines 0 comments Download
M src/mips/codegen-mips.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/mips/debug-mips.cc View 2 chunks +3 lines, -4 lines 0 comments Download
M src/mips/full-codegen-mips.cc View 5 chunks +10 lines, -10 lines 0 comments Download
M src/mips/lithium-codegen-mips.h View 1 chunk +1 line, -1 line 0 comments Download
M src/mips/lithium-codegen-mips.cc View 12 chunks +14 lines, -14 lines 0 comments Download
M src/mips/lithium-mips.h View 1 chunk +1 line, -1 line 0 comments Download
M src/mips/lithium-mips.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M src/mips/macro-assembler-mips.h View 2 chunks +4 lines, -4 lines 0 comments Download
M src/mips/macro-assembler-mips.cc View 24 chunks +32 lines, -31 lines 0 comments Download
M src/objects.h View 1 2 3 2 chunks +282 lines, -2 lines 0 comments Download
M src/objects.cc View 1 2 3 3 chunks +13 lines, -2 lines 0 comments Download
M src/x64/builtins-x64.cc View 7 chunks +10 lines, -10 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 20 chunks +23 lines, -25 lines 0 comments Download
M src/x64/codegen-x64.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/x64/full-codegen-x64.cc View 7 chunks +11 lines, -11 lines 0 comments Download
M src/x64/lithium-codegen-x64.h View 1 chunk +1 line, -1 line 0 comments Download
M src/x64/lithium-codegen-x64.cc View 4 chunks +4 lines, -4 lines 0 comments Download
M src/x64/lithium-x64.h View 1 chunk +1 line, -1 line 0 comments Download
M src/x64/lithium-x64.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M src/x64/macro-assembler-x64.h View 2 chunks +4 lines, -4 lines 0 comments Download
M src/x64/macro-assembler-x64.cc View 1 31 chunks +41 lines, -40 lines 0 comments Download
M src/x64/regexp-macro-assembler-x64.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/x64/stub-cache-x64.cc View 1 chunk +1 line, -1 line 0 comments Download
M test/cctest/test-code-stubs-ia32.cc View 1 chunk +1 line, -1 line 0 comments Download
M test/cctest/test-code-stubs-x64.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 10 (0 generated)
loislo
the most part of the patch is a mechanical changes that were made by the ...
7 years, 4 months ago (2013-08-01 09:52:04 UTC) #1
alph
https://codereview.chromium.org/20843012/diff/6001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/20843012/diff/6001/src/objects.cc#newcode15968 src/objects.cc:15968: ASSERT(reason >= kLastErrorMessage); < https://codereview.chromium.org/20843012/diff/6001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/20843012/diff/6001/src/objects.h#newcode1123 ...
7 years, 4 months ago (2013-08-01 10:35:50 UTC) #2
Jakob Kummerow
DBC. Could you explain briefly what the motivation for this change is? https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc File src/compiler.cc ...
7 years, 4 months ago (2013-08-01 12:02:13 UTC) #3
loislo
On 2013/08/01 12:02:13, Jakob wrote: > DBC. > > Could you explain briefly what the ...
7 years, 4 months ago (2013-08-01 12:27:59 UTC) #4
loislo
https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc#newcode488 src/compiler.cc:488: info()->set_bailout_reason(kCodeGenerationFailed); On 2013/08/01 12:02:13, Jakob wrote: > While you're ...
7 years, 4 months ago (2013-08-01 13:15:29 UTC) #5
Jakob Kummerow
LGTM with one more comment. https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc#newcode488 src/compiler.cc:488: if (info()->bailout_reason() != kNoReason) ...
7 years, 4 months ago (2013-08-02 08:13:46 UTC) #6
loislo
Committed patchset #4 manually as r16024 (presubmit successful).
7 years, 4 months ago (2013-08-02 09:53:43 UTC) #7
loislo
On 2013/08/02 08:13:46, Jakob wrote: > LGTM with one more comment. > > https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc > ...
7 years, 4 months ago (2013-08-02 09:54:51 UTC) #8
Jakob Kummerow
On 2013/08/02 09:54:51, loislo wrote: > On 2013/08/02 08:13:46, Jakob wrote: > > LGTM with ...
7 years, 4 months ago (2013-08-09 14:02:29 UTC) #9
yurys
7 years, 4 months ago (2013-08-09 15:09:26 UTC) #10
Message was sent while issue was closed.
On 2013/08/09 14:02:29, Jakob wrote:
> On 2013/08/02 09:54:51, loislo wrote:
> > On 2013/08/02 08:13:46, Jakob wrote:
> > > LGTM with one more comment.
> > > 
> > > https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc
> > > File src/compiler.cc (right):
> > > 
> > >
> https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc#newcode488
> > > src/compiler.cc:488: if (info()->bailout_reason() != kNoReason) {
> > > Sorry, I mixed up the condition in my earlier comment. Obviously this
should
> > be
> > > "==" -- only set the reason if it's not set yet.
> > 
> > done
> 
> I think you forgot to commit this change or something. bleeding_edge still
says:
> 
>       if (info()->bailout_reason() != kNoReason) {
>         info()->set_bailout_reason(kCodeGenerationFailed);
>       }
> 
> which is bogus.

Fix is up for review: https://codereview.chromium.org/22404007/

Powered by Google App Engine
This is Rietveld 408576698