664703 - Rename VMPI_alloca et al

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

VMPI_alloca is misnamed:

- its API is not that of alloca()
- it's not a VMPI level function
- VMPI_calloca (bug #656567) does not clear its memory, unlike calloc()

The proposal is to rename VMPI_alloca as avmStackAlloc (retaining the current API), rename VMPI_alloca_gc as avmStackAllocGC (ditto) and name the about-to-be introduced VMPI_calloca and VMPI_calloca_gc as avmStackAllocArray and avmStackAllocArrayGC, respectively.

Comment 1

[Lars T Hansen]

Attachment: Obvious patch

Comment 2

[Stan Switzer]

Please don't rename these things now; it collides with the work I am doing on the foundation utilities and it seems to carve out a new naming scheme in the porting later (avm...). This needs to be coordinated with me and Don Woodward.

Comment 3

[Steven Johnson]

Comment on attachment 539772 [details] [diff] [review]
Obvious patch

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

Changing to F+ for now, pending Stan and Lars working out a landing strategy.

Comment 4

[Stan Switzer]

The semantics issues are noted and should be fixed as indicated. Its a question of naming only. There's a fair chance we'll all be happier with a new naming scheme for VMPI_, but it almost certainly isn't avmXyz.

Earlier discussion here:
   https://bugzilla.mozilla.org/show_bug.cgi?id=645878#c13

Comment 5

[Lars T Hansen]

(In reply to comment #4)
> The semantics issues are noted and should be fixed as indicated. Its a
> question of naming only. There's a fair chance we'll all be happier with a
> new naming scheme for VMPI_, but it almost certainly isn't avmXyz.

What?

This isn't about any naming scheme at all, just a reflection that VMPI_alloca is not porting layer functionality and should never have been named VMPI_something at all.

Comment 6

[Stan Switzer]

Oops, sorry! Right. It does not collide (I was fooled by the name, which as you say was wrong in the first place).

No objections.

Comment 7

[Steven Johnson]

Comment on attachment 539772 [details] [diff] [review]
Obvious patch

Changing to R+.

Comment 8

[Lars T Hansen]

(In reply to comment #6)
> Oops, sorry! Right. It does not collide (I was fooled by the name, which as
> you say was wrong in the first place).
> 
> No objections.

Thanks.  I'll land this, it'll be good to get it cleaned up, it's a long-standing wart (and the larger payoff is the calloca version in bug #656567).  VMPI_alloca is used a couple of places in player code too, AFAIK.

Comment 9

[Tamarin Bot]

changeset: 6399:e2128bf9184d
user:      Lars T Hansen <[e-mail]>
summary:   Fix 664703 - Rename VMPI_alloca et al (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/e2128bf9184d