search

vtil-project / VTIL-Core

Virtual-machine Translation Intermediate Language
BSD 3-Clause "New" or "Revised" License
1.31k stars 165 forks source link

Fix ::min and ::max usage on Windows #46

Closed mrexodia closed 3 years ago

mrexodia commented 3 years ago

utils/numeric_iterator crashes MSVC without it

can1357 commented 3 years ago

Not too sure if this is an issue of the repo to be honest, the only place using Windows headers already defines this macro at https://github.com/vtil-project/VTIL-Core/blob/152a7e2355251d1651838d6e1d6665bc892cc5e2/VTIL-Common/io/logger.cpp#L31. Though I suppose I can merge it if you think we should protect from any headers user can include.

mrexodia commented 3 years ago

I’m not sure what the best solution is. I think of the NOMINMAX as a requirement to VTIL-Common, which is why I put it in the project as PUBLIC.

The issue I encountered is that you get internal compiler errors if you include Windows.h before some things and it’s not exactly obvious to a novice user that they have to define this...

On Mon, 26 Oct 2020 at 05:27, Can Bölük notifications@github.com wrote:

Not too sure if this is an issue of the repo to be honest, the only place using Windows headers already defines this macro at https://github.com/vtil-project/VTIL-Core/blob/152a7e2355251d1651838d6e1d6665bc892cc5e2/VTIL-Common/io/logger.cpp#L31. Though I suppose I can merge it if you think we should protect from any headers user can include.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/vtil-project/VTIL-Core/pull/46#issuecomment-716295987, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGJJ3OIZIPPFGGLXHJLSMT3EHANCNFSM4S4N6SOA .

mrexodia commented 3 years ago

Another solution is to ifdef/under min and max everywhere you use it in core so that it’s not necessary to define it.

Or create an option VTIL_DEFINE_NOMINMAX, or simply leave it as-is and let the user deal with the annoying issue (although tbh I think it’s better to be user friendly and somehow prevent/fix it)

On Mon, 26 Oct 2020 at 09:57, Duncan Ogilvie mr.exodia.tpodt@gmail.com wrote:

I’m not sure what the best solution is. I think of the NOMINMAX as a requirement to VTIL-Common, which is why I put it in the project as PUBLIC.

The issue I encountered is that you get internal compiler errors if you include Windows.h before some things and it’s not exactly obvious to a novice user that they have to define this...

On Mon, 26 Oct 2020 at 05:27, Can Bölük notifications@github.com wrote:

Not too sure if this is an issue of the repo to be honest, the only place using Windows headers already defines this macro at https://github.com/vtil-project/VTIL-Core/blob/152a7e2355251d1651838d6e1d6665bc892cc5e2/VTIL-Common/io/logger.cpp#L31. Though I suppose I can merge it if you think we should protect from any headers user can include.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/vtil-project/VTIL-Core/pull/46#issuecomment-716295987, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASYFGJJ3OIZIPPFGGLXHJLSMT3EHANCNFSM4S4N6SOA .

can1357 commented 3 years ago

Yeah you're right, I'll merge it. I honestly hate the windows headers so much for this stupid macro, feels bad fixing their shit for them on a completely unrelated repo just because I'd like to use the "Standard" library /shrug.