Closed Bug 664882 Opened 13 years ago Closed 13 years ago

IonMonkey: Add spew mechanism

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch spew (obsolete) — Splinter Review
Works like JM and TM:

dvander@dvander:~/mozilla/ionmonkey/js/src$ IONFLAGS=all ./Debug32/js Y.js 
[MIR] Analying script Y.js:9
[Abort] Unsupported opcode: 226 (line 10)
Attachment #539937 - Flags: review?(adrake)
Comment on attachment 539937 [details] [diff] [review]
spew

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

::: js/src/ion/IonBuilder.cpp
@@ +96,5 @@
> +IonBuilder::fail()
> +{
> +    if (error_)
> +        return false;
> +    return ABORT("out of memory");

If this is only for OOM situations, wouldn't outOfMemory or something other than "fail" be more specific?

::: js/src/ion/IonBuilder.h
@@ +162,5 @@
>      bool build();
>  
> +    // Prints a message about compilation failure, if none has been printed
> +    // yet.
> +    bool fail();

I guess this is technically true now, but it seems like this function should take a message or error code or something?

::: js/src/ion/IonSpew.cpp
@@ +212,5 @@
> +
> +            "  all      Everything\n"
> +            "\n"
> +        );
> +        exit(0);

If this were exit(1) it would be a little more obvious that we took an abnormal way out of the engine, but it doesn't necessarily indicate failure. Thoughts?

@@ +224,5 @@
> +        LoggingBits = uint32(-1);
> +}
> +
> +void
> +ion::IonSpewFmt(IonSpewChannel channel, const char *fmt, va_list ap)

I'd probably call this IonSpewVA or something that indicates that it takes a va_list, rather than Fmt which implies to me that it takes a format string, which is true of just IonSpew too.

::: js/src/ion/MIRGraph.h
@@ +115,5 @@
>      }
>  
> +    // Set an error state and prints a message. Returns false so errors can be
> +    // propagated up.
> +    bool ABORT(const char *message, ...);

Having this be SHOUTY makes it stand out a bit too much and makes me think it's a macro when it isn't. Can it just be "abort"?
Attached patch v2Splinter Review
addresses comments
Attachment #539937 - Attachment is obsolete: true
Attachment #539937 - Flags: review?(adrake)
Attachment #541164 - Flags: review?(adrake)
Attachment #541164 - Flags: review?(adrake) → review+
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/9e51c35ab223
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.