Refactor C++ namespace usage

[mgreter]

Add new sass.hpp header for minimal std includes and exporting only specific symbols from std into our Sass namespace. Also removed some redundant includes! This is an alternative to @saper changes, which blindly adds std:: to every symbol. I opted to only do this for more esoteric symbols and leave it as is for the more common ones.

Main fix is that I got rid of all using namespace std by explicitly saying:

namespace Sass {

  // export only some symbols from std
  // avoids to expose full std namespace
  using std::cerr;
  using std::endl;
  using std::stack;
  using std::vector;
  using std::string;
  using std::stringstream;
  using std::runtime_error;

}

Those are the only symbols imported into the Sass namespace. You can then import that namespace if you want. IMO this should solve the most important problem, which was that we were exporting all symbols in std into the global namespace in multiple headers!

I guess I don't have to emphasise that I like this solution much better than the approach in #1310.
It should also keeps the git history much cleaner!

[saper]

I don't recall needing to add std::stack anywhere... (#include <stack> can safely go away as well)

[saper]

Maybe we can get rid of the File namespace at all (if it makes making bindings more difficult...)? Also Util then...

[mgreter]

Bindings are only in the C space, nothing from outside should interface with the C++ internals! There is no stable API on that side!!

[saper]

I hope it is not necessary, haven't tried swig for years...

[saper]

Re git history: It got much more complicated with the file moves, so I wouldn't worry too much about it now.

For example:

m> git diff namespaces ^master -- values.hpp 
diff --git a/values.hpp b/values.hpp
new file mode 100644
index 0000000..765ed86
--- /dev/null
+++ b/values.hpp
@@ -0,0 +1,12 @@
+#ifndef SASS_VALUES_H
+#define SASS_VALUES_H
+
+#include "ast.hpp"
+
+namespace Sass {
+
+  union Sass_Value* ast_node_to_sass_value (const Expression* val);
+  Value* sass_value_to_ast_node (Memory_Manager<AST_Node>& mem, const union Sass_Value* val);
+
+}
+#endif

[mgreter]

I mean mostly for ie. blame commands, since it changed a lot of existing lines. From looking at the commits, it looks like values.hpp file was added and removed before (2012), but I also don't see an actual remove commit. But from the changes it is clear that there was one in between! The files actually was newly added with the source-move commit!

[mgreter]

Resolved the nitpicks ... I'm pretty sure there are much more redundant header includes. But IMHO It is just not really worth the time to hunt them all down!

[saper]

It was meant to be part of a "lightweight drop-in" library :) and moved in from src to root in e74a31d

[xzyfer]

I have no specific feedback. I'm more interested in @saper's thoughts.

[drewwells]

👍

[mgreter]

Would like to move forward with this so we can prepare the first beta releases!
@saper do you think this is good enough to solve the initial issue?

[saper]

Ok, it's my turn. This is going to be difficult because it is mostly a matter of style but it also has some technical implications (like keeping namespace pollution to the minimum).

1. I don't think that having std:: in almost every place required is a bad thing. I kind of got used to it very quickly and since I am browsing other C++ projects like V8 or Chromium - they use consequently prefixes almost everywhere.
2. Having std::string or std::hash actually makes code sometimes more readable. I found few places where it helps:
   • many places where we have string to_string. std::string to_string is more readable to me
   • we've had a local variable called string somewhere (

     libsass/src/ast.hpp

     Line 1362 in 7ed376f

     for (auto string : elements())

     ) - that's okay if the standard type is always std::string.
   • places like

     libsass/src/ast.hpp

     Lines 932 to 937 in 7ed376f

     	if (hash_ == 0) {
     	hash_ = std::hash<size_t>()(type_);
     	hash_combine(hash_, left()->hash());
     	hash_combine(hash_, right()->hash());
     	}
     	return hash_;

     - I don't know STL by heart and initially I thought hash_combine was some standard library function. If we have a policy to put namespace prefixes everywhere, there is no doubt: every item from the library is namespaced by std::. Rare exceptions could be made with using only within some crowded code (we don't have that many).
3. Google Style Guide is something one does not have to follow (they have some very strict decisions on some items), but they are right in pointing out that using namespace should never be used and using can be used only in .cpp files (or within methods in the header files).
4. I strongly dislike introduction of one more header file for this purpose. Also including using std::string in the header files, like using std::string; in sass2scss.h. The reason is pollution again: when I was removing using clauses from source files for Clean up C++ namespaces #1310 there was always one more header file where the using clause was hiding. So from this point view, a header file makes no sense - it's just a pure redundancy on one hand - but if you don't do rendudancy, then it's obscure magic. You put using into some obscure header file (say sass2scss.h) and those using clauses magically operate everywhere. If you have them explicit at the top of the file and, say, want to remove them - they still magically work. You remove #include "sass.h" and they still work, because they are hidden in some other headerfile. It took me 13 commits in Clean up C++ namespaces #1310 before I realized that the using clause was still hiding somewhere (in saper@324f3b3).

That's why I think there is no easy way out - we need to get it out of headers, use std:: almost everywhere. We can for example put using std::string etc. at the top of the most busy .cpp files (in #1310 I decided to leave it in 6 source files: https://github.com/saper/libsass/blob/namespaces/file.cpp#L38, https://github.com/saper/libsass/blob/namespaces/parser.cpp#L22-L23 etc., but more I think about it I would propose a rule to include std:: always in the new code and slowly modify existing code to replace existing uses gradually.

Unfortunately, the headers need to be dealt with now and uniformly, as we don't have long methods there which would justify putting using std::string inside them.

I am not using an editor that magically completes identifiers - and I still prefer to type those std::s by hand.

[mgreter]

So here my short feedback:

1. I guess it's already clear that my personal opinion is against needing std:: for most common types.

2. Probably true, but IMHO really just minor things and there are probably counter arguments!

3. IMO it should be valid to import stuff into our own namespace (which is what the new header is doing, importing std::string and a few others into Sass namespace. I agree we should not pollute global namespace via any headers, IMHO it's a different story for cpp files!

4. We could include this in another header, we don't have to introduce a new one. But IMO it very easily shows you what we import by default from the std namespace into the Sass namespace. So if you are using Sass namespace, you know what to expect IMHO. Also sass2scss.h now imports the symbols into the Sass namespace, so no global pollution!?

I will be pretty much away for 2 weeks now!

[xzyfer]

I've been following all the discussion around this, and giving it some thought. I'm in agreement with @saper here. The minor overhead of typing std is massively outweighed but he improved code clarity it provides.

On a personal note: I'm not a career c++ dev. As a result I've learned on the fly by googling. To me the explicitness std is a huge help.

On a community note: This project is gain massive popularity and our contributor base is going. Sticking to some form a best practice or styleguide will be a big boost to helping new contributors get up to speed. Things are much better than they where a year ago, and IMHO explicit namespacing makes things better again.