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

Strict c++17 now compiles #13

Closed
wants to merge 9 commits into from

Conversation

Nielsbishere
Copy link
Contributor

@Nielsbishere Nielsbishere commented Dec 4, 2019

Fixes issue #11

Now detecting if memcpy and memset are actually needed. And same goes for the vertex check (they check correctly now).

Paq.sh now automatically outputs to nuklear.h.

… for vertices (they check correctly now).

Paq.sh now automatically outputs to nuklear.h.
@dumblob
Copy link
Member

dumblob commented Dec 4, 2019

Continuing discussion from #11:

Why is it needed to introduce yet another define? Couldn't we use #ifdef NK_MEMSET and #ifdef NK_MEMCPY directly as mentioned in #11 (comment) ?

@Nielsbishere
Copy link
Contributor Author

Because the moment that ifdef is used, it is immediately defined.

@dumblob
Copy link
Member

dumblob commented Dec 4, 2019

I mean, c89 supports #ifdef NK_MEMSET == nk_memset...

@Nielsbishere
Copy link
Contributor Author

Does it? C++17 gave me an error on that.

@dumblob
Copy link
Member

dumblob commented Dec 5, 2019

Right, nk_memset would get expanded to 0, so that won't work. In that case, could you please rename NK_DEFAULT_MEMCPY to NK_MEMCPY_BUILTIN and NK_DEFAULT_MEMSET to NK_MEMSET_BUILTIN? It's usually better to use the same prefix if chaining anything.

@dumblob
Copy link
Member

dumblob commented Dec 5, 2019

And because it changes nuklear.h, we should also bump version.

@Nielsbishere
Copy link
Contributor Author

Okay, I'll get on it when I got time again

@Nielsbishere
Copy link
Contributor Author

Nielsbishere commented Dec 5, 2019

Fixed. @dumblob

@dumblob
Copy link
Member

dumblob commented Dec 9, 2019

One last little piece - the version info has to be updated also in https://github.com/Immediate-Mode-UI/Nuklear/blob/master/src/CHANGELOG 😉 (and nuklear.h regenerated).

@Nielsbishere
Copy link
Contributor Author

Done.

@Nielsbishere
Copy link
Contributor Author

Nielsbishere commented Dec 11, 2019

Merged with the latest PR and updated so it works with strict clang c++17 too.

@dumblob
Copy link
Member

dumblob commented Dec 12, 2019

I'm sorry, didn't notice it before, but could you make paq.bat behave in a way, that it outputs no CRLF, but just LF line endings? nuklear.h was always with LF and I'd like it to stay so. Now this PR is huge just because it changes all LF to CRLF, which I'm hesitant to merge.

Thanks again for your effort!

@Nielsbishere
Copy link
Contributor Author

Nielsbishere commented Dec 12, 2019

Fixed @dumblob, python used 'print' which prints newline characters based on the current platform. I set it to force linux line endings.

@Hejsil
Copy link
Contributor

Hejsil commented Mar 5, 2020

LGTM. Maybe just squash the commits into one, to keep the git history tidy :)

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.

None yet

3 participants