[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,13 +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 sourceloc(const SrcLoc& loc) { m_srcloc = loc; }
 
  void sourceline(int line) { m_sourceline = line; }
 
@@ -407,8 +405,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 +442,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 +473,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;