Odin icon indicating copy to clipboard operation
Odin copied to clipboard

Failure to index contant array

Open SaculRennorb opened this issue 1 year ago • 2 comments

I'm not quite sure if this should be a bug report or feature request.

Context

Odin: dev-2024-01-nightly:5961d4b3 OS: Windows 10 Enterprise N (version: 20H2), build 19042.1466 CPU: Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz RAM: 65470 MiB

Failure Information (for bugs)

Cannot index a constant 'CRC_TABLE'
        crc ~= CRC_TABLE[temp]
Suggestion: store the constant into a variable in order to index it with a variable index

Steps to Reproduce

CRC_TABLE :: [256]u16 { /* usually there would be values here, but for brevity I cut them out*/ }

CRC16 :: proc(data : []u8, previous : u16 = 0xffff) -> u16
{
	crc := previous

	for b in data {
		temp := (byte)(u16(b) ~ crc)
		crc >>= 8
		crc ~= CRC_TABLE[temp]
	}

	return crc
}

The compiler does suggest a 'fix':

CRC16 :: proc(data : []u8, previous : u16 = 0xffff) -> u16
{
	crc := previous
+	crc_table := CRC_TABLE

	for b in data {
		temp := (byte)(u16(b) ~ crc)
		crc >>= 8
-		crc ~= CRC_TABLE[temp]
+		crc ~= crc_table[temp]
	}

	return crc
}

and I have read #2495 , but I don't quite follow the logic there (it's also a slightly different case).

Constants get baked into the executable, things like

init_header :: proc() -> Header
{
	return Header {
		control_code = HEADER_CONTROLL_CODE_REQUEST,
	}
}

HEADER_CONTROLL_CODE_REQUEST : u16le : 0x4510

do work.

The only thing required to index a constant array is its element width (and size for bounds checks), so I se no reason why you shouldn't be able to index a constant array directly, without having to assign it to a variable first.

I've also read #659 , although one might argue that in that case the index is constant and therefore its a different story.

If this is intentional (one could argue around the read only nature of consts and there not being any difference between read and write access for indexing operations), some clarifications in the documentation (maybe here ) would be highly appreciated. I'm also guessing that there are similar reasons for disallowing taking the pointer to a const element, but even after reading the two documents liked from the overview section I could not find a solid reason against at least the indexing case.

SaculRennorb avatar Jan 12 '24 17:01 SaculRennorb

TL;DR: just use a variable instead. Long explanation follows.

Constants get baked into the executable

This is not true of Odin at all. Usage of constants does get inlined, for example if one uses an integer constant, the compilation will behave as if the value of the constant was used directly. Note that unlike variables you will not be able to inspect values of constants after the program has been compiled, e.g. with objdump. You can know which variables there are though.

In C, const specifies that a given binding is immutable, so you will not be able to alter its binding again. Such variable exists at runtime, it's just not alterable.

Odin differs from that perception in that a constant means "a compile-time constant". Such value does not exist at runtime, but has a reusable value. You can not index them, nor you can do other things that you can't do to other constants -- like take the address of their value or assign to them.

If Odin did allow constants to be indexable, think about this:

CRC_TABLE[temp] = 3 // assigning to constant?
addr := (transmute(^u8) &CRC_TABLE[temp])^ // taking an address?

Accounting for these kinds of things in the compiler would slightly complicate the compiler for not much benefit.

Also, I would like to address a case from your example:

for b in data {
    temp := (byte)(u16(b) ~ crc)
    crc >>= 8
    crc ~= CRC_TABLE[temp]
}

In this example you're addressing a compile-time value that exists exclusively during compile-time using a run-time value whose value isn't known at compile-time. Allowing this is simply impossible to be implemented.

Since Odin doesnt have the concept of mutability and at the end it's all variables anyway I would suggest you to use a variable. It will have all the same behaviors you are desiring, and there's no reason to specify it's immutability.

Also if you pass that array as parameter to procedure funnily enough it will become immutable.

flysand7 avatar Jan 13 '24 10:01 flysand7

Hmm. Ok, I obviously somewhat misunderstood the idea behind const, but I think I might have a better idea about it by now. I understand that just using a variable here solves the 'issue', but for the sake of debate; odin not embedding but the const (array) is straight up wrong - even though I must concede that the example I gave before will in fact most likely be inlined (which ofcourse does make sense in that case, and any other reasonable compiler would do the same).

Without looking into the inner workings of the compiler directly; If you compile the example I gave with actual data in the array, you can just open the binary and find the exact bytes in there. Not interwoven with individual move instructions to put them on some stack memory, but the whole block at once (which is ofc the reasonable thing to do in this usecase, so nothing wrong there). That means the compiler already does a lot of what would be required (in terms of emitting) to make the direct indexing work, its just seems to be missing the famous last 10%.

I still think my proposal could be a nice bit of qol, but I do however also fully understand that there are arguments against the idea, namely it adding complexity for little benefit, and since the request mostly came from a place of misunderstanding I do not have any problem with it just getting dismissed. I will however not close this issue immediately, just so the idea might get seen.

SaculRennorb avatar Jan 13 '24 10:01 SaculRennorb

After using the language for quite a while now i remembered this issue and have to say that my problem was 100% a missing understanding of how constants work. I still think the docs could be more direct / clear about this, but it is not something that should be changed on the compiler side.

SaculRennorb avatar Jan 15 '25 14:01 SaculRennorb