search

meltwater / served

A C++11 RESTful web server library
MIT License
710 stars 174 forks source link

WIP: Change enum style to prevent name clashes #35

Closed jpihl closed 5 years ago

jpihl commented 6 years ago

Hi again :)

The library now builds on Windows and with the Android NDK toolchain ( BTW, I also successfully tested on iOS, but I didn't need to make any changes for that to work).

Unfortunately I had to change the style of the enums, which is a rather disruptive change. The reason for this is that certain windows headers (annoyingly) defines the MACROs ERROR and DELETE. Another way of fixing this issue would be to use the directive #undef, but to me it seems unsafe and intrusive to undefine such macros - I guess they could be used somewhere.

I would also like to change status.hpp to be an enum and then use this new style for consistency - but that is a bit tedious work, so I will only do it if you like the idea :)

Let me know what you think.

Jeffail commented 6 years ago

Hey @jpihl, I'd prefer to avoid breaking API changes at this stage. I agree that the #undef approach is unsafe and dirty though. I'm not sure what a clean, backwards compatible solution would look like. Unfortunately I don't have a windows machine available to test this out with either.

jpihl commented 6 years ago

It's a very annoying issue. I don't know what the people defining these MACROs were thinking.

If you define NOGDI you can remove the ERROR MACRO, but I haven't found a way to remove the DELETE MACRO (other than using #undef). And I guess defining NOGDI could also have some potentially unwanted repercussions. Also the users of this library would have to define it as well since the enums are in the header files.

I've researched this problem quiet a bit and, to me, renaming the enums is the only real solution if Windows support is needed (which is it is to me).

Also I would like to note that none of the examples are using the enums (nor the constants in status.hpp), so this change may not cause any issues for users of this library. You are of course more qualified to make that guess :)

mortenvp commented 6 years ago

I also took a look at this and it seems fighting windows.h pollution will be an uphill battle. As also noted here: https://stackoverflow.com/a/1377968/1717320 it seems that renaming to avoid clashes is the pragmatic way to go.. although it would break backwards compatibility I think it is a better long term solution.

jpihl commented 5 years ago

Hi @Jeffail, any update on this? Do you still value non-breaking API changes over Windows support?

Edit: Sorry for spamming this message - GitHub were having some issues.

Jeffail commented 5 years ago

Hey @jpihl, going to close this one for now. If I ever get time to build a Served V2 API then I'll take these changes into account as it'd be nice to support Windows.