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

Prefix constants with a prefix to prevent collisions #1802

Closed
xzyfer opened this issue Dec 27, 2015 · 9 comments
Closed

Prefix constants with a prefix to prevent collisions #1802

xzyfer opened this issue Dec 27, 2015 · 9 comments

Comments

@xzyfer
Copy link
Contributor

xzyfer commented Dec 27, 2015

I think we should just rename all constants and prefix them with something like LIBSASS_ to avoid issues like the SEC one. Otherwise it'll be the endless chase...
#1782 (comment).

I believe we start prefixing some constants with SASS_ 3.3. We should finish the job.

@xzyfer xzyfer added this to the 3.4 milestone Dec 27, 2015
@mgreter
Copy link
Contributor

mgreter commented Jan 9, 2016

IMO it is okay as it is now. So far only C-API stuff is prefixed with SASS_, since we have a Sass namespace on the C++ side. The issues we had were due to using namespace std which has been dropped, so it should be ok now. I really would like to encourage to work with namespaces in C++ (i.e. Units now live in SassUnit namespace). There will always be compiler quirks as ie. the one with std::round that surfaced on cygwin when we prefixed all identifiers from std.

Edit: According to google the correct way to avoid these non-standard defines/macros (as the SEC macro on smartos), we could set _POSIX_C_SOURCE before including any system header files. I suspect that the SEC problems stems from time.h and this should solve it, but no idea how that plays together with the c++11 stuff and various compilers.

I really don't think we need to to overly pro-active here.

@mgreter mgreter removed this from the 3.4 milestone Jan 9, 2016
@saper
Copy link
Member

saper commented Jan 10, 2016

What about just having LIBSASS_ prefix on all our constants?

C++ namespaces will not solve SEC issue afaik (the preprocessor will still try to expand it!)

@mgreter
Copy link
Contributor

mgreter commented Jan 10, 2016

I absolutely would hate that, but I guess that's already clear. So we would have Sass::SassUnit::LIBSASS_SEC just because one compiler system can't keep it together? I say let's keep it as it is with the #undef SEC since that macro should never be defined in the first place!

@saper
Copy link
Member

saper commented Jan 10, 2016

It is not the compiler, those are the header files. If you don't like it we can drop SassUnit, since that's a bit too much. it's a known problem in the C world and most libraries try to namespace like this, this is nothing unusual. Windows is also adding random symbols defined here and there, you might by lucky with one SDK or version setting, but not with another. We can't change a whole world.

@QuLogic
Copy link
Contributor

QuLogic commented Jan 10, 2016

Is the LIB part really necessary though?

@mgreter
Copy link
Contributor

mgreter commented Jan 10, 2016

So I took the pleasure to install smartos on a virtual machine, which took me 3 hours due to a bug in their installer 👊 Once I had it running it took me another hour to figure out how to install gcc (pkgin in scmgit-base gcc47; export PATH="$PATH:/opt/local/gcc47/bin/"). Once I was that far it took me a couple of minutes to figure out how to avoid these macros to be exported:

In src/units.cpp:

#undef __EXTENSIONS__
#include <stdexcept>
#include "units.hpp"

This makes all #undef obsolete and the file compiles correctly. This needs to be place into every compile unit before any system header includes. This is much easier to apply and I think we should create a common sass.hpp header that must be the first include in every compilation unit.

Will try to find out what that __EXTENSIONS__ define actually is ...

@mgreter
Copy link
Contributor

mgreter commented Jan 10, 2016

@QuLogic we already export stuff under the SASS_ prefixes with the C-API, so mixing C and C++ here would even be worse, since we ie. already have a SASS_STRING or SASS_ERROR on the C-API.

@mgreter
Copy link
Contributor

mgreter commented Jan 10, 2016

I have create a PR which fixes the issue when I test it on SunOS smartos 5.11 joyent_20160108T173524Z i86pc i386 i86pc. I suspect this bug or some similar stuff going on:

C++ options for C++0x / GNU-extended C++0x (-std=c++0x, -std=gnu++0x)
should probably be treated like those for c99/gnu99, since C++0x uses the
C99 library (whereas C++98/C++03 uses the C90+AMD1 library).

We pass -std=c++0x so it should probably not use any gnu extensions!?
Still have no idea why __EXTENSIONS__ is defined by the compiler on solaris??

@mgreter
Copy link
Contributor

mgreter commented Jan 14, 2016

I'm closing this as it should not be needed. I jumped through hoops to ensure this is fixed now!

@mgreter mgreter closed this as completed Jan 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants