[oslcomp] Reworked source location to include column numbers

and track begin/end pairs. Introduce OSL::pvt::SrcLoc to track this and
percolate throughout the source

Signed-off-by: Luca Fascione <lfascione@nvidia.com>

Author: lucafascione

src/include/osl_pvt.h

@@ -15,6 +15,111 @@ namespace pvt {
 class ASTNode;
 class StructSpec;
 
+/// A location in the OSL source
+/// Conversions from the FLEX/Bison convention happens in from_yyloc()
+/// It's tempting to make this class also track the straight byte-offset
+/// in the file, which would help us when we print out errors, code in the
+/// OSO's and whathaveyou. But the action of the preprocessor kills our
+/// ability to do so, as the parser doesn't ever see the user's version
+/// of the source
+struct SrcLoc {
+    static const uint32_t kUnknown { uint32_t(-1) };
+
+    ustring filename;
+    // this storage asymmetry is weird, but it's what you want:
+    // the column markers work like a normal STL [begin, end) span,
+    // but the line markers are actually [start, stop] because they
+    // need to say on which line the non-inclusive `column_end` side is
+    uint32_t line_start = kUnknown;  ///< start line number, 0-based, inclusive
+    uint32_t column_begin
+        = kUnknown;  ///< start col number, 0-based, [begin, end)-style like the STL
+    uint32_t line_stop = kUnknown;  ///< finish line number, 0-based, inclusive
+    uint32_t column_end
+        = kUnknown;  ///< finish col number, 0-based, [begin, end)-style like the STL
+
+    SrcLoc() = default;
+
+    SrcLoc(ustring filename, int first_line, int first_column, int last_line, int last_column) :
+        filename(filename)
+    {
+        from_yyloc(first_line, first_column, last_line, last_column);
+    }
+
+    explicit operator bool() const { return !filename.empty(); }
+
+    /// Convert the classic/built-in representation of a YYLTYPE
+    /// to our convention
+    void from_yyloc(int first_line, int first_column, int last_line,
+                    int last_column)
+    {
+        // we go from 1-based to 0-based
+        line_start   = first_line   ? first_line - 1   : kUnknown;
+        column_begin = first_column ? first_column - 1 : kUnknown;
+        line_stop    = last_line    ? last_line - 1    : kUnknown;
+        column_end   = last_column  ? last_column - 1  : kUnknown;
+    }
+
+    bool operator==(const SrcLoc& other) const
+    {
+        return line_start == other.line_start
+               && column_begin == other.column_begin
+               && line_stop == other.line_stop
+               && column_end == other.column_end;
+    }
+
+    bool operator!=(const SrcLoc& other) const { return !(*this == other); }
+
+    /// Operator < compares the start locations only.
+    /// This is what you want, trust me
+    bool operator<(const SrcLoc& other) const
+    {
+        return line_start < other.line_start
+               || (line_start == other.line_start
+                   && column_begin < other.column_begin);
+    }
+
+    /// How many lines spanned by this token.
+    /// Unlike colcount(), which needs care to be used, this is always correct.
+    /// Also, unlike colcount(), this is probably nowhere near as useful
+    size_t linecount() const { return line_stop - line_start + 1; }
+
+    /// How many columns spanned by this token.
+    /// N.B. This needs to be used with care: first off it is only ever a correct
+    /// notion when first_lineno() == last_lineno(), because we don't know how
+    /// long lines are. Further, the parser counts all whitespace as _one_ character
+    /// so '\t' counts as one, not 8 (or 4, or whatever your terminal is set to)
+    size_t colcount() const { return column_end - column_begin; }
+
+    /// Line number, 1-based, for humans, first line of the location span, inclusive
+    int first_lineno() const { return line_start + 1; }
+
+    /// Column number, 1-based, for humans, first line of the location span, inclusive
+    int first_colno() const { return column_begin + 1; }
+
+    /// Line number, 1-based, for humans, last line of the location span, inclusive
+    int last_lineno() const { return line_stop + 1; }
+
+    /// Column number, 1-based, for humans, last character of the location span, inclusive
+    // (because of our internal representation this does not need a "+1")
+    int last_colno() const { return column_end; }
+
+    // see also fmt::formatter at the end of the file
+    friend std::ostream& operator<<(std::ostream& out, const SrcLoc& sl)
+    {
+        if (sl.column_begin != kUnknown)
+            return out << fmtformat("{}:{}:{}", sl.filename, sl.last_lineno(),
+                                    sl.last_colno());
+        else if (sl.line_start != kUnknown)
+            return out << fmtformat("{}:{}", sl.filename, sl.last_lineno());
+        else if (!sl.filename.empty())
+            return out << fmtformat("{}", sl.filename);
+
+        // if there is no filename we print nothing
+        return out;
+    }
+};
+
+
 
 /// Kinds of shaders
 ///
@@ -937,7 +1042,7 @@ typedef std::vector<Symbol*> SymbolPtrVec;
 class Opcode {
 public:
     Opcode(ustring op, ustring method, size_t firstarg = 0, size_t nargs = 0)
-        : m_firstarg((int)firstarg), m_method(method), m_sourceline(0)
+        : m_firstarg((int)firstarg), m_method(method)
     {
         reset(op, nargs);  // does most of the heavy lifting
     }
@@ -959,13 +1064,11 @@ class Opcode {
     int nargs() const { return m_nargs; }
     ustring method() const { return m_method; }
     void method(ustring method) { m_method = method; }
-    void source(ustring sourcefile, int sourceline)
-    {
-        m_sourcefile = sourcefile;
-        m_sourceline = sourceline;
-    }
-    ustring sourcefile() const { return m_sourcefile; }
-    int sourceline() const { return m_sourceline; }
+    void source(const SrcLoc& srcloc) { m_srcloc = srcloc; }
+    ustring sourcefile() const { return m_srcloc.filename; }
+    const SrcLoc& srcloc() const { return m_srcloc; }
+    // removeme
+    int sourceline() const { return m_srcloc.first_lineno(); }
 
     void set_args(size_t firstarg, size_t nargs)
     {
@@ -1131,8 +1234,7 @@ class Opcode {
     int m_nargs;                    ///< Total number of arguments
     ustring m_method;               ///< Which param or method this code is for
     int m_jump[max_jumps];          ///< Jump addresses (-1 means none)
-    ustring m_sourcefile;           ///< Source filename for this op
-    int m_sourceline;               ///< Line of source code for this op
+    SrcLoc m_srcloc;                ///< Location in source code for this op
     unsigned int m_argread;         ///< Bit field - which args are read
     unsigned int m_argwrite;        ///< Bit field - which args are written
     unsigned int m_argtakesderivs;  ///< Bit field - which args take derivs
@@ -1172,5 +1274,6 @@ OSL_NAMESPACE_END
 #if FMT_VERSION >= 100000
 FMT_BEGIN_NAMESPACE
 template<> struct formatter<OSL::pvt::TypeSpec> : ostream_formatter {};
+template<> struct formatter<OSL::pvt::SrcLoc> : ostream_formatter {};
 FMT_END_NAMESPACE
 #endif

src/liboslcomp/ast.cpp

@@ -71,8 +71,7 @@ reverse(ASTNode::ref list)
 ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler)
     : m_nodetype(nodetype)
     , m_compiler(compiler)
-    , m_sourcefile(compiler->filename())
-    , m_sourceline(compiler->lineno())
+    , m_srcloc(compiler->srcloc())
     , m_op(0)
     , m_is_lvalue(false)
 {
@@ -88,8 +87,7 @@ ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler, int op,
                  ASTNode* a)
     : m_nodetype(nodetype)
     , m_compiler(compiler)
-    , m_sourcefile(compiler->filename())
-    , m_sourceline(compiler->lineno())
+    , m_srcloc(compiler->srcloc())
     , m_op(op)
     , m_is_lvalue(false)
 {
@@ -105,8 +103,7 @@ ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler, int op,
 ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler, int op)
     : m_nodetype(nodetype)
     , m_compiler(compiler)
-    , m_sourcefile(compiler->filename())
-    , m_sourceline(compiler->lineno())
+    , m_srcloc(compiler->srcloc())
     , m_op(op)
     , m_is_lvalue(false)
 {
@@ -122,8 +119,7 @@ ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler, int op,
                  ASTNode* a, ASTNode* b)
     : m_nodetype(nodetype)
     , m_compiler(compiler)
-    , m_sourcefile(compiler->filename())
-    , m_sourceline(compiler->lineno())
+    , m_srcloc(compiler->srcloc())
     , m_op(op)
     , m_is_lvalue(false)
 {
@@ -141,8 +137,7 @@ ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler, int op,
                  ASTNode* a, ASTNode* b, ASTNode* c)
     : m_nodetype(nodetype)
     , m_compiler(compiler)
-    , m_sourcefile(compiler->filename())
-    , m_sourceline(compiler->lineno())
+    , m_srcloc(compiler->srcloc())
     , m_op(op)
     , m_is_lvalue(false)
 {
@@ -161,8 +156,7 @@ ASTNode::ASTNode(NodeType nodetype, OSLCompilerImpl* compiler, int op,
                  ASTNode* a, ASTNode* b, ASTNode* c, ASTNode* d)
     : m_nodetype(nodetype)
     , m_compiler(compiler)
-    , m_sourcefile(compiler->filename())
-    , m_sourceline(compiler->lineno())
+    , m_srcloc(compiler->srcloc())
     , m_op(op)
     , m_is_lvalue(false)
 {
@@ -213,31 +207,31 @@ ASTNode::~ASTNode()
 void
 ASTNode::error_impl(string_view msg) const
 {
-    m_compiler->errorfmt(sourcefile(), sourceline(), "{}", msg);
+    m_compiler->errorfmt(sourceloc(), "{}", msg);
 }
 
 
 
 void
 ASTNode::warning_impl(string_view msg) const
 {
-    m_compiler->warningfmt(sourcefile(), sourceline(), "{}", msg);
+    m_compiler->warningfmt(sourceloc(), "{}", msg);
 }
 
 
 
 void
 ASTNode::info_impl(string_view msg) const
 {
-    m_compiler->infofmt(sourcefile(), sourceline(), "{}", msg);
+    m_compiler->infofmt(sourceloc(), "{}", msg);
 }
 
 
 
 void
 ASTNode::message_impl(string_view msg) const
 {
-    m_compiler->messagefmt(sourcefile(), sourceline(), "{}", msg);
+    m_compiler->messagefmt(sourceloc(), "{}", msg);
 }
 
 
@@ -366,7 +360,7 @@ ASTfunction_declaration::ASTfunction_declaration(OSLCompilerImpl* comp,
                                                  TypeSpec type, ustring name,
                                                  ASTNode* form, ASTNode* stmts,
                                                  ASTNode* meta,
-                                                 int sourceline_start)
+                                                 const SrcLoc& srcloc_start)
     : ASTNode(function_declaration_node, comp, 0, meta, form, stmts)
     , m_name(name)
     , m_sym(NULL)
@@ -375,8 +369,12 @@ ASTfunction_declaration::ASTfunction_declaration(OSLCompilerImpl* comp,
     // Some trickery -- the compiler's idea of the "current" source line
     // is the END of the function body, so if a hint was passed about the
     // start of the declaration, substitute that.
-    if (sourceline_start >= 0)
-        m_sourceline = sourceline_start;
+    // FIXME: [lfascione] Maybe with the move from int sourceline to SrcLoc this
+    // is the right thing to do here
+    if (srcloc_start) {
+        m_srcloc.line_start   = srcloc_start.line_start;
+        m_srcloc.column_begin = srcloc_start.column_begin;
+    }
 
     if (Strutil::starts_with(name, "___"))
         errorfmt("\"{}\" : sorry, can't start with three underscores", name);
@@ -386,7 +384,9 @@ ASTfunction_declaration::ASTfunction_declaration(OSLCompilerImpl* comp,
     if (existing_syms && existing_syms->symtype() != SymTypeFunction) {
         errorfmt("\"{}\" already declared in this scope as a {}", name,
                  existing_syms->typespec());
-        // FIXME -- print the file and line of the other definition
+        // print the file and line of the other definition when available
+        if (existing_syms->node())
+            comp->message_srcloc(existing_syms->node()->sourceloc());
         existing_syms = NULL;
     }
 
@@ -429,13 +429,9 @@ ASTfunction_declaration::ASTfunction_declaration(OSLCompilerImpl* comp,
                     }
                     err += "\n    ";
                     if (other) {
-                        err += Strutil::fmt::format(
-                            "{}:{}",
-                            OIIO::Filesystem::filename(
-                                other->sourcefile().string()),
-                            other->sourceline());
+                        err += Strutil::fmt::format("{}", other->sourceloc());
                     } else
-                        err += "built-in";
+                        err += "(built-in)";
                 }
             }
         }
@@ -519,7 +515,7 @@ ASTvariable_declaration::ASTvariable_declaration(OSLCompilerImpl* comp,
                                                  ustring name, ASTNode* init,
                                                  bool isparam, bool ismeta,
                                                  bool isoutput, bool initlist,
-                                                 int sourceline_start)
+                                                 const SrcLoc& srcloc_start)
     : ASTNode(variable_declaration_node, comp, 0, init, NULL /* meta */)
     , m_name(name)
     , m_sym(NULL)
@@ -531,8 +527,10 @@ ASTvariable_declaration::ASTvariable_declaration(OSLCompilerImpl* comp,
     // Some trickery -- the compiler's idea of the "current" source line
     // is the END of the declaration, so if a hint was passed about the
     // start of the declaration, substitute that.
-    if (sourceline_start >= 0)
-        m_sourceline = sourceline_start;
+    if (srcloc_start) {
+        m_srcloc.line_start   = srcloc_start.line_start;
+        m_srcloc.column_begin = srcloc_start.column_begin;
+    }
 
     if (m_initlist && init) {
         // Typecheck the init list early.
@@ -547,10 +545,8 @@ ASTvariable_declaration::ASTvariable_declaration(OSLCompilerImpl* comp,
             = Strutil::fmt::format("\"{}\" already declared in this scope",
                                    name);
         if (f->node()) {
-            std::string filename = OIIO::Filesystem::filename(
-                f->node()->sourcefile().string());
-            e += Strutil::fmt::format("\n\t\tprevious declaration was at {}:{}",
-                                      filename, f->node()->sourceline());
+            e += Strutil::fmt::format("\n\t\tprevious declaration was at {}",
+                                      f->node()->sourceloc());
         }
         if (f->scope() == 0 && f->symtype() == SymTypeFunction && isparam) {
             // special case: only a warning for param to mask global function

src/liboslcomp/ast.h

@@ -191,15 +191,11 @@ class ASTNode : public OIIO::RefCnt {
     /// Reverse the order of the list.
     friend ASTNode::ref reverse(ASTNode::ref list);
 
-    /// What source file was this parse node created from?
+    /// What location in the source file was this parse node created from?
     ///
-    ustring sourcefile() const { return m_sourcefile; }
+    const SrcLoc sourceloc() const { return m_srcloc; }
 
-    /// What line of the source file was this parse node created from?
-    ///
-    int sourceline() const { return m_sourceline; }
-
-    void sourceline(int line) { m_sourceline = line; }
+    void sourceloc(const SrcLoc& loc) { m_srcloc = loc; }
 
     template<typename... Args>
     void errorfmt(const char* format, const Args&... args) const
@@ -407,8 +403,7 @@ class ASTNode : public OIIO::RefCnt {
     NodeType m_nodetype;          ///< Type of node this is
     ref m_next;                   ///< Next node in the list
     OSLCompilerImpl* m_compiler;  ///< Back-pointer to the compiler
-    ustring m_sourcefile;  ///< Filename of source where the node came from
-    int m_sourceline;      ///< Line number in source where the node came from
+    SrcLoc m_srcloc;              ///< Location in source where the node came from
     std::vector<ref> m_children;  ///< Child nodes
     int m_op;                     ///< Operator selection
     TypeSpec m_typespec;          ///< Data type of this node
@@ -445,7 +440,7 @@ class ASTfunction_declaration final : public ASTNode {
 public:
     ASTfunction_declaration(OSLCompilerImpl* comp, TypeSpec type, ustring name,
                             ASTNode* form, ASTNode* stmts, ASTNode* meta,
-                            int sourceline_start = -1);
+                            const SrcLoc& srcloc_start);
     const char* nodetypename() const { return "function_declaration"; }
     const char* childname(size_t i) const;
     void print(std::ostream& out, int indentlevel = 0) const;
@@ -476,7 +471,7 @@ class ASTvariable_declaration final : public ASTNode {
     ASTvariable_declaration(OSLCompilerImpl* comp, const TypeSpec& type,
                             ustring name, ASTNode* init, bool isparam,
                             bool ismeta, bool isoutput, bool initlist,
-                            int sourceline_start = -1);
+                            const SrcLoc& srcloc_start);
     const char* nodetypename() const;
     const char* childname(size_t i) const;
     void print(std::ostream& out, int indentlevel = 0) const;