Generic live trace facility

[saper]

This patch series introduces a live tracing facility (enabled by setting LIBSASS_TRACE environment variable to any value) which enables detailed verbose logging via TRACE() and TRACEINST() macros.

This facility is used here to troubleshoot issue with casting of String_Quoted/String_Contant types.

[mgreter]

IMHO you could've used debug_ast.

#include "debugger.hpp"
String_Quoted* s = new ...
debug_ast(s, "message: ");

Output might be a bit crypric for some state details. And it's still evolving, so it may not yet handle everything!

[saper]

Well the problem is that debug_ast is probably affected by the very problem I am debugging: it uses dynamic_cast<> which seems to not do the right thing for the strings.

I am more interested in tracing lifecycle of certain objects - when they are created and used.

From the example dump (It also includes output from https://github.com/sass/node-sass/pull/1007):

[LIBSASS] 0x803c83600:operator() output.cpp:391 This should be a constant string... (0,"list-style-type")
[LIBSASS] 0x803c83600:operator() output.cpp:393 ... but dynamic_cast<String_Quoted*> worked
[LIBSASS] 0x803c83600:operator() output.cpp:376 This should be a quoted string... (0,"list-style-type")
[LIBSASS] 0x803c83600:operator() output.cpp:381 ... no quote mark(?), sending via string_to_output

It seems that void Output::operator()(String_Constant* s) gets called first always, and not the the operator for the derived type.... The code looks very strange - on one hand we depend on the runtime type information, but on the other hand - we always check for quoting...

[mgreter]

@saper Not sure how good you're knowledge is about the CRTP pattern libsass uses. The reason why void Output::operator()(String_Constant* s) is called is a "bug" in operations.hpp, which is missing a default visitor for String_Quoted. I fixed this just now in https://github.com/mgreter/libsass/commit/f1938e5e95ba9a2a092419ce51d6016ef8523b10

[saper]

Thanks. Will look it up. So now we can get rid of this pretty confusing constructs like

if (dynamic cast...) {
  do something
} else if (quote_mark) { 
  do something
} else { 
  definitely_not_quoted
}

?

[saper]

@mgreter Ok, I read a little on CRTP and frankly I don't see it applied there .... we just use plain old polymorphism (with virtual tables) and run time type information, both with their overhead.

Can you elaborate a bit?

[mgreter]

There is an ATTACH_OPERATIONS defined in ast_def_macros.hpp which is added to every AST_Node class. This sets up the perform visitor. I'm no expert at this either, but I guess due to the missing visitor in operations.hpp the compiler was only able to find the most common visitor for String_Quoted which was indeed String_Constant. In there we have a static_cast which probably is the reason why only that visitor gets called (and I guess the dynamic_cast we had there fixed this at runtime).

[saper]

Thanks! With CRTP we could actually get rid of dynamic cast, and break output.cpp into output methods for respective types... that would be awesome

[saper]

Mini-documentation: https://github.com/sass/libsass/wiki/Live-trace-facility