Skip to content

[oslcomp] Reworked source location to include column numbers #2006

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[oslcomp] Reworked source location to include column numbers #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 113 additions & 10 deletions src/include/osl_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
62 changes: 29 additions & 33 deletions src/liboslcomp/ast.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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)
{
Expand All @@ -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)
{
Expand All @@ -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)
{
Expand All @@ -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)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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)
Expand All @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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)";
}
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand All @@ -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
Expand Down
17 changes: 6 additions & 11 deletions src/liboslcomp/ast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Loading
Loading