531888 - Overzealous compilers don't like Assembler::intersectRegisterState

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Steven Johnson]

This funtion contains the statement

        for (Register r=LastReg; 
             r >= FirstReg && r <= LastReg;
             r = prevreg(r))

which generates

    error: comparison is always true due to limited range of data type

on current android versions of gcc, causing spurious build failures.

Comment 1

[Rick Reitmaier]

I guess we could put NanoAssert(r <= LastReg); in the body and remove the condition from the check.

Comment 2

[Steven Johnson]

Attachment: Patch

One possible patch (fixes it). Open for more elegant solutions.

Comment 3

[Rick Reitmaier]

Comment on attachment 415184 [details] [diff] [review]
Patch

Fix looks fine (albeit ugly), but do we know the source of the warning?  Can we just tweak the comparison(s) and add any appropriate asserts.

Comment 4

[Steven Johnson]

(In reply to comment #3)
> (From update of attachment 415184 [details] [diff] [review])
> Fix looks fine (albeit ugly), but do we know the source of the warning?  Can we
> just tweak the comparison(s) and add any appropriate asserts.

Source is that Register can't (legally) ever be outside that range; prevreg/nextreg hack things to make that possible, but the compiler can't tell that.

Should I get a mozillan to review this too?

Comment 5

[Rick Reitmaier]

I see and yes having Nick look at it would be worthwhile.  

I'd prefer not to push this change (maybe I should have marked it '-'?) unless we are truly forced.

Comment 6

[Steven Johnson]

Comment on attachment 415184 [details] [diff] [review]
Patch

Adding Nick (or do you prefer Nicholas?) ... it's an ugly fix I'm not particularly happy with, so I'm open for better suggestions.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 415184 [details] [diff] [review]
Patch

"Nick" is fine.

This is ugly but at least it has a comment.  Although I'd appreciate a capital letter at the start of the comment and a full stop at the end :)

Comment 8

[Steven Johnson]

Pretty sure that this has been pushed to all relevant repos, shall we close it?