Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid impl dependent multi-char constants #1178

Merged
merged 2 commits into from
Oct 16, 2018
Merged

Avoid impl dependent multi-char constants #1178

merged 2 commits into from
Oct 16, 2018

Conversation

cengiz-io
Copy link
Contributor

Hello!

I was receiving this warning while compiling for linux:

src/host/premake.c: In function ‘premake_init’:
src/host/premake.c:197:36: warning: multi-character character constant [-Wmultichar]
  lua_rawseti(L, LUA_REGISTRYINDEX, 'SHIM');
                                    ^~~~~~

Instead of disabling the warning, I've decided to convert that array key to
an expression.

And I've tested with following compilers:

gcc version 7.3.0 (Ubuntu 7.3.0-27ubuntu1~18.04)
gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)

Thank you

Hello!

I was receiving this warning while compiling for linux:

```
src/host/premake.c: In function ‘premake_init’:
src/host/premake.c:197:36: warning: multi-character character constant [-Wmultichar]
  lua_rawseti(L, LUA_REGISTRYINDEX, 'SHIM');
                                    ^~~~~~
```

Instead of disabling the warning, I've decided to convert that array key to
an expression.

And I've tested with following compilers:

gcc version 7.3.0 (Ubuntu 7.3.0-27ubuntu1~18.04)
gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)

Thank you
@WorldofBay
Copy link
Contributor

WorldofBay commented Oct 15, 2018

You change the array index from 1397246285 to 305 for gcc, clang, msvc and icc. I doubt that this points at the correct value.

const Node* n = findNode(reg,((((('S' << 8) + 'H') << 8) + 'I') << 8) + 'M');

See here

@cengiz-io
Copy link
Contributor Author

Good point!

However I don't think the actual value has any effect on node retrieval.

What I read from code, we're creating a node with a fixed index key and retrieving it afterwards.

Unless of course, the value was chosen to be something that won't collide with other node indexes that are < 1.3m.

@samsinsane
Copy link
Member

I'd prefer that this was changed to the hex value, 0x5348494D with a comment indicating that it's equivalent to 'SHIM'.

The value is important, existing binary modules use 'SHIM' and won't work if Premake5 is using 'S' + 'H' + 'I' + 'M'. The current change will require that all existing binary modules be rebuilt against the latest version of luashim, we have no way of knowing how many binary modules our users have, if any. It's best to avoid breaking them where we can.

@samsinsane samsinsane merged commit 367b227 into premake:master Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants