696067 - avmfeatures.as should generate #warning if feature switches are already defined

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Build Config, defect)

Description

[Edwin Smith]

The protocol for features is that the vm host defines feature or system switches, then the features define VMCFG_ (etc) names that control functionality.  It is wrong for any of those secondary switches to be already defined, upon entering the top of avmfeatures.h

We could add two safeguards:

1. for every switch defined by every feature, guard that its not already defined, #error out if so.

2. add an <obsolete> section for legacy switches that nobody defines, but that are permanently unused.

Comment 1

[Edwin Smith]

Attachment: #warn if any configuration output switches are defined before avmfeatures.h

This can't land as-is, because it breaks everything; our own build scripts define things they shouldn't, notably AVMPLUS_UNIX.

Comment 2

[Edwin Smith]

I also have found player code switching on input switches (AVMFEATURE_x, AVMSYSTEM_x), or using them with #ifdef or defined() rather than normal logic.

from a casual grep, these need looking at:

#if AVMFEATURE_OSR                      - settings code
!defined(AVMFEATURE_AOT_DISABLE_NEON)   - rendering code, many hits
defined (AVMFEATURE_MEMORY_PROFILER)    - android

Comment 3

[Edwin Smith]

(In reply to Edwin Smith from comment #2)

> !defined(AVMFEATURE_AOT_DISABLE_NEON)   - rendering code, many hits

This one isn't even defined by core/avmfeatures.as, so we can ignore it,
but using the name "AVMFEATURE_XXX" outside avmfeatures.as is probably
asking for trouble.

Comment 4

[Lars T Hansen]

(In reply to Edwin Smith from comment #3)
> (In reply to Edwin Smith from comment #2)
> 
> > !defined(AVMFEATURE_AOT_DISABLE_NEON)   - rendering code, many hits
> 
> This one isn't even defined by core/avmfeatures.as, so we can ignore it,
> but using the name "AVMFEATURE_XXX" outside avmfeatures.as is probably
> asking for trouble.

We should crack down on this.

Plus, this looks like a tweak more than a feature so it's triply bad.

And it's a "negative" name (disable_blahblah) so quadruply bad.