Skip to content

Commit

Permalink
Improve parser interface
Browse files Browse the repository at this point in the history 
Make more methods `const` by doing work in the constructors of certain
`*Reader` classes.  This means that we don't have to worry as much about
calling `name()` before `value()` in the `VariableReader`, which is hard
to do if you are calling `foo(v.name(), v.value())` as there's no C++
guarantee which function is called first.

Add structured bindings to `VariableReader` to keep the interface even
more succinct.

Rename non-`const` methods to `read*()` so that it is more obvious that
they are mutating.
  • Loading branch information
@elliotgoodrich
elliotgoodrich committed Nov 24, 2024
1 parent ec0772c commit 37057dd
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 124 deletions.
50 changes: 18 additions & 32 deletions src/builddirutil.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,9 @@ struct BuildDirContext {

BuildDirContext() = default;

static void consume(PathRangeReader&& range) {
for ([[maybe_unused]] const EvalString& r : range) {
}
}

static void consume(LetRangeReader&& range) {
for (VariableReader&& r : range) {
[[maybe_unused]] const std::string_view name = r.name();
[[maybe_unused]] const EvalString& value = r.value();
template <typename RANGE>
static void consume(RANGE&& range) {
for ([[maybe_unused]] auto&& _ : range) {
}
}

Expand All @@ -58,35 +52,28 @@ struct BuildDirContext {
}
}

void operator()(PoolReader& r) const {
[[maybe_unused]] const std::string_view name = r.name();
consume(r.variables());
}
void operator()(PoolReader& r) const { consume(r.readVariables()); }

void operator()(BuildReader& r) const {
consume(r.out());
consume(r.implicitOut());
[[maybe_unused]] const std::string_view ruleName = r.name();
consume(r.in());
consume(r.implicitIn());
consume(r.orderOnlyDeps());
consume(r.validations());
consume(r.variables());
consume(r.readOut());
consume(r.readImplicitOut());
[[maybe_unused]] const std::string_view ruleName = r.readName();
consume(r.readIn());
consume(r.readImplicitIn());
consume(r.readOrderOnlyDeps());
consume(r.readValidations());
consume(r.readVariables());
}

void operator()(RuleReader& r) const {
[[maybe_unused]] const std::string_view name = r.name();
consume(r.variables());
}
void operator()(RuleReader& r) const { consume(r.readVariables()); }

void operator()(DefaultReader& r) const { consume(r.paths()); }
void operator()(DefaultReader& r) const { consume(r.readPaths()); }

void operator()(VariableReader& r) {
const std::string_view name = r.name();
evaluate(fileScope.resetValue(name), r.value(), fileScope);
void operator()(const VariableReader& r) {
evaluate(fileScope.resetValue(r.name()), r.value(), fileScope);
}

void operator()(IncludeReader& r) {
void operator()(const IncludeReader& r) {
const std::filesystem::path file = [&] {
const EvalString& pathEval = r.path();
std::string path;
Expand All @@ -107,10 +94,9 @@ struct BuildDirContext {
parse(file, ninjaCopy.str());
}

void operator()(SubninjaReader& r) const {
void operator()(const SubninjaReader&) const {
// subninja introduces a new scope so we can never modify the top-level
// `builddir` variable
[[maybe_unused]] const EvalString& p = r.path();
}
};

Expand Down
82 changes: 54 additions & 28 deletions src/manifestparser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,27 @@ std::size_t BaseReaderWithStart::bytesParsed() const {

} // namespace detail

std::string_view VariableReader::name() {
std::string_view name;
if (!m_lexer->ReadIdent(&name)) {
VariableReader::VariableReader(Lexer* lexer,
EvalString* storage,
const char* start)
: detail::BaseReaderWithStart{lexer, storage, start}, m_name{} {
if (!m_lexer->ReadIdent(&m_name)) {
throw std::runtime_error("Missing variable name");
}
return name;
}

const EvalString& VariableReader::value() {
expectToken(m_lexer, Lexer::EQUALS);

std::string err;
m_storage->clear();
if (!m_lexer->ReadVarValue(m_storage, &err)) {
throw std::runtime_error(err);
}
}

std::string_view VariableReader::name() const {
return m_name;
}

const EvalString& VariableReader::value() const {
return *m_storage;
}

Expand Down Expand Up @@ -194,30 +199,36 @@ PathRangeReader::sentinel PathRangeReader::end() const {
return sentinel{};
}

std::string_view PoolReader::name() {
std::string_view name;
if (!m_lexer->ReadIdent(&name)) {
PoolReader::PoolReader()
: detail::BaseReaderWithStart{nullptr, nullptr, nullptr}, m_name{} {}

PoolReader::PoolReader(Lexer* lexer, EvalString* storage, const char* start)
: detail::BaseReaderWithStart{lexer, storage, start}, m_name{} {
if (!m_lexer->ReadIdent(&m_name)) {
throw std::runtime_error("Missing name for pool");
}

expectToken(m_lexer, Lexer::NEWLINE);
return name;
}

LetRangeReader PoolReader::variables() {
std::string_view PoolReader::name() const {
return m_name;
}

LetRangeReader PoolReader::readVariables() {
return LetRangeReader{m_lexer, m_storage};
};

PathRangeReader BuildReader::out() {
PathRangeReader BuildReader::readOut() {
return PathRangeReader{m_lexer, m_storage};
}

PathRangeReader BuildReader::implicitOut() {
PathRangeReader BuildReader::readImplicitOut() {
return m_lexer->PeekToken(Lexer::PIPE) ? PathRangeReader{m_lexer, m_storage}
: PathRangeReader{};
}

std::string_view BuildReader::name() {
std::string_view BuildReader::readName() {
expectToken(m_lexer, Lexer::COLON);
std::string_view name;
if (!m_lexer->ReadIdent(&name)) {
Expand All @@ -226,69 +237,85 @@ std::string_view BuildReader::name() {
return name;
}

PathRangeReader BuildReader::in() {
PathRangeReader BuildReader::readIn() {
return PathRangeReader{m_lexer, m_storage};
}

PathRangeReader BuildReader::implicitIn() {
PathRangeReader BuildReader::readImplicitIn() {
return m_lexer->PeekToken(Lexer::PIPE) ? PathRangeReader{m_lexer, m_storage}
: PathRangeReader{};
}

PathRangeReader BuildReader::orderOnlyDeps() {
PathRangeReader BuildReader::readOrderOnlyDeps() {
return m_lexer->PeekToken(Lexer::PIPE2) ? PathRangeReader{m_lexer, m_storage}
: PathRangeReader{};
}

PathRangeReader BuildReader::validations() {
PathRangeReader BuildReader::readValidations() {
return m_lexer->PeekToken(Lexer::PIPEAT) ? PathRangeReader{m_lexer, m_storage}
: PathRangeReader{};
}

LetRangeReader BuildReader::variables() {
LetRangeReader BuildReader::readVariables() {
expectToken(m_lexer, Lexer::NEWLINE);
return LetRangeReader{m_lexer, m_storage};
}

std::string_view RuleReader::name() {
RuleReader::RuleReader(Lexer* lexer, EvalString* storage, const char* start)
: detail::BaseReaderWithStart{lexer, storage, start}, m_name{} {
std::string_view name;
if (!m_lexer->ReadIdent(&name)) {
throw std::runtime_error("Missing name for rule");
}

expectToken(m_lexer, Lexer::NEWLINE);
return name;
}

LetRangeReader RuleReader::variables() {
std::string_view RuleReader::name() const {
return m_name;
}

LetRangeReader RuleReader::readVariables() {
return LetRangeReader{m_lexer, m_storage};
};

PathRangeReader DefaultReader::paths() {
PathRangeReader DefaultReader::readPaths() {
return PathRangeReader{m_lexer, m_storage, Lexer::NEWLINE};
}

const EvalString& IncludeReader::path() {
IncludeReader::IncludeReader(Lexer* lexer,
EvalString* storage,
const char* start)
: detail::BaseReaderWithStart{lexer, storage, start} {
std::string err;
m_storage->clear();
if (!m_lexer->ReadPath(m_storage, &err)) {
throw std::runtime_error(err);
}
expectToken(m_lexer, Lexer::NEWLINE);
}

const EvalString& IncludeReader::path() const {
return *m_storage;
}

const std::filesystem::path& IncludeReader::parent() const {
return m_lexer->getFilename();
}

const EvalString& SubninjaReader::path() {
SubninjaReader::SubninjaReader(Lexer* lexer,
EvalString* storage,
const char* start)
: detail::BaseReaderWithStart{lexer, storage, start} {
std::string err;
m_storage->clear();
if (!m_lexer->ReadPath(m_storage, &err)) {
throw std::runtime_error(err);
}
expectToken(m_lexer, Lexer::NEWLINE);
}

const EvalString& SubninjaReader::path() const {
return *m_storage;
}

Expand All @@ -297,8 +324,7 @@ const std::filesystem::path& SubninjaReader::parent() const {
}

ManifestReader::iterator::iterator(Lexer* lexer, EvalString* storage)
: detail::BaseReader{lexer, storage},
m_value{PoolReader{nullptr, nullptr, nullptr}} {
: detail::BaseReader{lexer, storage}, m_value{PoolReader{}} {
// We need to set `m_value` to anything so just choose `PoolReader`
++*this;
}
Expand Down
Loading

0 comments on commit 37057dd

Please sign in to comment.