521328 - TM: make the arena code less ugh

Status: RESOLVED DUPLICATE of bug 684039

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Igor Bukanov]

Comment on attachment 405373 [details] [diff] [review]
patch

>+template <typename T>
>+static inline void
>+JS_ARENA_ALLOCATE_COMMON(T& p, JSArenaPool *pool, size_t nb)
>+{

I suggest to use standard camel case for all arena functions with few #defines for compatibility with the current code.

>+static inline jsuword
>+JS_UPTRDIFF(void *p, void *q)
>+{
>+    return ((jsuword)(p) - (jsuword)(q));
>+}

>+
>+static inline bool
>+JS_ARENA_MARK_MATCH(JSArena *a, void *mark)
>+{
>+    return JS_UPTRDIFF(mark, (void *)a->base) <= JS_UPTRDIFF((void *)a->avail, (void *)a->base);
>+}


The casts in UPTRDIFF users look ugly. I suggest to turn JSArena fields into uint8 * to minimize the amount of casts. r+ with that addressed.

Comment 5

[Andreas Gal :gal]

I will leave the ugly all caps names in there for now, but I agree they should go. I will make the arena a class with methods in a follow-up patch (and then fix the uses too).

I will fix the other nit as suggested.

Comment 6

[Andreas Gal :gal]

Actually I would prefer to leave it at jsuword for now. Changing to uint8 causes a large number of changes all over the place (alignment code). A lot of it is in macros and pretty brittle. I would like to change this in smaller steps with the class-ification and method-ification patch. Are you ok with an r+ as is? This first patch really just tries to make most macros go away.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Actually I would prefer to leave it at jsuword for now. Changing to uint8
> causes a large number of changes all over the place (alignment code). A lot of
> it is in macros and pretty brittle. I would like to change this in smaller
> steps with the class-ification and method-ification patch. Are you ok with an
> r+ as is?

Sorry I have missed that comment - bugzilla should have an option to auto CC for review comments.

I do not like extra casts due to turning JS_UPTRDIFF into an inline function which makes few its users more ugly. What about keeping that as a macro for now and adding an inline that does not require a cast in a followup bug?

Comment 8

[Andreas Gal :gal]

Ok, I don't mind keeping it as a macro. I will upload a new patch.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

jsarena was revamped in bug 684039. DUPE or WONTFIX this one?

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Thanks Ryan!