Skip to content
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.

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

Add Dump functions to Check, Parse, and Lex #4669

Merged
merged 22 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
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
5 changes: 3 additions & 2 deletions common/ostream.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
#ifndef CARBON_COMMON_OSTREAM_H_
#define CARBON_COMMON_OSTREAM_H_

// Libraries should include this header instead of raw_ostream.

#include <concepts>
#include <ostream>
#include <type_traits>

#include "llvm/Support/raw_os_ostream.h"
// Libraries should include this header instead of raw_ostream.
#include "llvm/Support/Compiler.h"
#include "llvm/Support/raw_os_ostream.h"
#include "llvm/Support/raw_ostream.h" // IWYU pragma: export

namespace Carbon {
Expand Down
20 changes: 20 additions & 0 deletions toolchain/check/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ cc_library(
"//common:array_stack",
"//common:check",
"//common:map",
"//common:ostream",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer used in this PR and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing

"//common:vlog",
"//toolchain/base:index_base",
"//toolchain/base:kind_switch",
Expand All @@ -90,6 +91,24 @@ cc_library(
"//toolchain/sem_ir:typed_insts",
"@llvm-project//llvm:Support",
],
alwayslink = 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment why this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this one was missed, I also wondered if it's actually needed since dump was moved to another cc_library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I wasn't looking closely enough because I just assumed I'd already removed this set. Removed.

)

cc_library(
name = "dump_id",
srcs = ["dump_id.cpp"],
copts = ["-Wno-missing-prototypes"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment why this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

deps = [
":context",
"//common:check",
"//common:ostream",
"//toolchain/lex:dump_id",
"//toolchain/lex:tokenized_buffer",
"//toolchain/parse:dump_id",
"//toolchain/parse:tree",
"//toolchain/sem_ir:file",
],
alwayslink = 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment why this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

)

cc_library(
Expand All @@ -109,6 +128,7 @@ cc_library(
hdrs = ["check.h"],
deps = [
":context",
":dump_id",
":impl",
":interface",
":pointer_dereference",
Expand Down
10 changes: 8 additions & 2 deletions toolchain/check/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define CARBON_TOOLCHAIN_CHECK_CONTEXT_H_

#include "common/map.h"
#include "common/ostream.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer used in this PR and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing

#include "llvm/ADT/FoldingSet.h"
#include "llvm/ADT/SmallVector.h"
#include "toolchain/check/decl_introducer_state.h"
Expand Down Expand Up @@ -483,10 +484,15 @@ class Context {
}

auto sem_ir() -> SemIR::File& { return *sem_ir_; }
auto sem_ir() const -> const SemIR::File& { return *sem_ir_; }

auto parse_tree() -> const Parse::Tree& { return sem_ir_->parse_tree(); }
auto parse_tree() const -> const Parse::Tree& {
return sem_ir_->parse_tree();
}

auto tokens() -> const Lex::TokenizedBuffer& { return parse_tree().tokens(); }
auto tokens() const -> const Lex::TokenizedBuffer& {
return parse_tree().tokens();
}

auto node_stack() -> NodeStack& { return node_stack_; }

Expand Down
66 changes: 66 additions & 0 deletions toolchain/check/dump_id.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef NDEBUG

#include "toolchain/lex/dump_id.h"

#include "common/check.h"
#include "common/ostream.h"
#include "toolchain/check/context.h"
#include "toolchain/lex/tokenized_buffer.h"
#include "toolchain/parse/dump_id.h"
#include "toolchain/parse/tree.h"
#include "toolchain/sem_ir/file.h"

namespace Carbon::Check {

static auto DumpIdImpl(const Context& context, SemIR::LocId loc_id) -> void {
if (!loc_id.is_valid()) {
llvm::errs() << "LocId(invalid)";
return;
}

if (loc_id.is_node_id()) {
auto token = context.parse_tree().node_token(loc_id.node_id());
auto line = context.tokens().GetLineNumber(token);
auto col = context.tokens().GetColumnNumber(token);
const char* implicit = loc_id.is_implicit() ? " implicit" : "";
llvm::errs() << "LocId(";
llvm::errs().write_escaped(context.sem_ir().filename());
llvm::errs() << ":" << line << ":" << col << implicit << ")";
} else {
CARBON_CHECK(loc_id.is_import_ir_inst_id());

auto import_ir_id = context.sem_ir()
.import_ir_insts()
.Get(loc_id.import_ir_inst_id())
.ir_id;
const auto* import_file =
context.sem_ir().import_irs().Get(import_ir_id).sem_ir;
llvm::errs() << "LocId(import from \"";
llvm::errs().write_escaped(import_file->filename());
llvm::errs() << "\")";
}
}

// A set of DumpId() overloads that dump an object to stderr, useful for
// calling inside a debugger.
LLVM_DUMP_METHOD auto DumpId(const Context& context, Lex::TokenIndex token)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says "this provides Check::Dump(context, arg) and similar" which is a typo (no Id) that we keep making. And we'll want to dump things other than Ids (like PendingBlock). So maybe we should drop the Id suffix. This is also fine for now, but I'm not happy with the naming forever yet. I was also talking with Geoff about maybe renaming "Dump" in Printable (Dump->Print and Print->PrintTo?) or something to avoid the name collisions.

Copy link
Contributor Author

@jonmeow jonmeow Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM typically calls it dump, which is where the name Dump came from:

https://github.com/search?q=repo%3Allvm%2Fllvm-project%20LLVM_DUMP_METHOD&type=code

(ed: to be clear, I think there's significant value in consistency)

You're correct the PR description doesn't match, I'll change the call though.

Copy link
Contributor Author

@jonmeow jonmeow Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, removing Dump from Printable would seem fine to me (I only added it for debugging, for others), I'm just meaning that if there's going to be a function that does dumping, it should probably use the familiar name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right yeah I don't want to diverge from LLVM too much either. My full opinion state here is that I'd prefer Dump to be used for the place that includes as much info as possible. The dump() in LLVM does include comprehensive amount of info generally everything you need when debugging, similarly. I'd personally be fine with renaming or removing the Dump function from Printable. I haven't had a lot of use of calling Dump explicitly on Printable types, as they are already streamable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to be sure, I'm assuming this'll be resolved separately)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, me too

-> void {
Lex::DumpId(context.tokens(), token);
}
LLVM_DUMP_METHOD auto DumpId(const Context& context, Parse::NodeId node_id)
-> void {
Parse::DumpId(context.parse_tree(), node_id);
}
LLVM_DUMP_METHOD auto DumpId(const Context& context, SemIR::LocId loc_id)
-> void {
DumpIdImpl(context, loc_id);
llvm::errs() << '\n';
}

} // namespace Carbon::Check

#endif // NDEBUG
12 changes: 12 additions & 0 deletions toolchain/lex/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ cc_library(
hdrs = ["lex.h"],
deps = [
":character_set",
":dump_id",
":helpers",
":numeric_literal",
":string_literal",
Expand All @@ -198,6 +199,17 @@ cc_library(
],
)

cc_library(
name = "dump_id",
srcs = ["dump_id.cpp"],
hdrs = ["dump_id.h"],
deps = [
":tokenized_buffer",
"//common:ostream",
],
alwayslink = 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment why this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

)

cc_library(
name = "token_index",
hdrs = ["token_index.h"],
Expand Down
38 changes: 38 additions & 0 deletions toolchain/lex/dump_id.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef NDEBUG

#include "toolchain/lex/dump_id.h"

#include "common/ostream.h"

namespace Carbon::Lex {

auto DumpIdImpl(const TokenizedBuffer& tokens, TokenIndex token) -> void {
if (!token.is_valid()) {
llvm::errs() << "TokenIndex(invalid)";
return;
}

auto kind = tokens.GetKind(token);
auto line = tokens.GetLineNumber(token);
auto col = tokens.GetColumnNumber(token);

llvm::errs() << "TokenIndex(kind: ";
kind.Print(llvm::errs());
llvm::errs() << ", loc: ";
llvm::errs().write_escaped(tokens.source().filename());
llvm::errs() << ":" << line << ":" << col << ")";
}

LLVM_DUMP_METHOD auto DumpId(const Lex::TokenizedBuffer& tokens,
Lex::TokenIndex token) -> void {
DumpIdImpl(tokens, token);
llvm::errs() << '\n';
}

} // namespace Carbon::Lex

#endif // NDEBUG
26 changes: 26 additions & 0 deletions toolchain/lex/dump_id.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef CARBON_TOOLCHAIN_LEX_DUMP_ID_H_
#define CARBON_TOOLCHAIN_LEX_DUMP_ID_H_

#ifndef NDEBUG

#include "toolchain/lex/tokenized_buffer.h"

namespace Carbon::Lex {

class TokenizedBuffer;

auto DumpIdImpl(const TokenizedBuffer& tokens, TokenIndex token) -> void;

// A set of DumpId() overloads that dump an object to stderr, useful for
// calling inside a debugger.
auto DumpId(const Lex::TokenizedBuffer& tokens, Lex::TokenIndex token) -> void;

} // namespace Carbon::Lex

#endif // NDEBUG

#endif // CARBON_TOOLCHAIN_LEX_DUMP_ID_H_
8 changes: 2 additions & 6 deletions toolchain/lex/tokenized_buffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ auto TokenizedBuffer::GetLine(TokenIndex token) const -> LineIndex {
}

auto TokenizedBuffer::GetLineNumber(TokenIndex token) const -> int {
return GetLineNumber(GetLine(token));
return GetLine(token).index + 1;
}

auto TokenizedBuffer::GetColumnNumber(TokenIndex token) const -> int {
Expand Down Expand Up @@ -162,10 +162,6 @@ auto TokenizedBuffer::IsRecoveryToken(TokenIndex token) const -> bool {
return recovery_tokens_[token.index];
}

auto TokenizedBuffer::GetLineNumber(LineIndex line) const -> int {
return line.index + 1;
}

auto TokenizedBuffer::GetNextLine(LineIndex line) const -> LineIndex {
LineIndex next(line.index + 1);
CARBON_DCHECK(static_cast<size_t>(next.index) < line_infos_.size());
Expand Down Expand Up @@ -262,7 +258,7 @@ auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream,
llvm::right_justify(
llvm::formatv("'{0}'", token_info.kind().name()).str(),
widths.kind + 2),
llvm::format_decimal(GetLineNumber(GetLine(token)), widths.line),
llvm::format_decimal(GetLineNumber(token), widths.line),
llvm::format_decimal(GetColumnNumber(token), widths.column),
llvm::format_decimal(GetIndentColumnNumber(line_index), widths.indent),
token_text);
Expand Down
3 changes: 0 additions & 3 deletions toolchain/lex/tokenized_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,6 @@ class TokenizedBuffer : public Printable<TokenizedBuffer> {
// For example, a closing paren inserted to match an unmatched paren.
auto IsRecoveryToken(TokenIndex token) const -> bool;

// Returns the 1-based line number.
auto GetLineNumber(LineIndex line) const -> int;

// Returns the 1-based indentation column number.
auto GetIndentColumnNumber(LineIndex line) const -> int;

Expand Down
13 changes: 13 additions & 0 deletions toolchain/parse/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ cc_library(
],
deps = [
":context",
":dump_id",
":node_kind",
":state",
":tree",
Expand All @@ -102,6 +103,18 @@ cc_library(
],
)

cc_library(
name = "dump_id",
srcs = ["dump_id.cpp"],
hdrs = ["dump_id.h"],
deps = [
":tree",
"//common:ostream",
"//toolchain/lex:dump_id",
],
alwayslink = 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment why this is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

)

cc_library(
name = "state",
srcs = ["state.cpp"],
Expand Down
2 changes: 1 addition & 1 deletion toolchain/parse/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ auto Context::PrintForStackDump(llvm::raw_ostream& output) const -> void {

auto Context::PrintTokenForStackDump(llvm::raw_ostream& output,
Lex::TokenIndex token) const -> void {
output << " @ " << tokens_->GetLineNumber(tokens_->GetLine(token)) << ":"
output << " @ " << tokens_->GetLineNumber(token) << ":"
<< tokens_->GetColumnNumber(token) << ": token " << token << " : "
<< tokens_->GetKind(token) << "\n";
}
Expand Down
45 changes: 45 additions & 0 deletions toolchain/parse/dump_id.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef NDEBUG

#include "toolchain/parse/dump_id.h"

#include "common/ostream.h"
#include "toolchain/lex/dump_id.h"

namespace Carbon::Parse {

auto DumpIdImpl(const Tree& tree, NodeId node_id) -> void {
if (!node_id.is_valid()) {
llvm::errs() << "NodeId(invalid)";
return;
}

auto kind = tree.node_kind(node_id);
auto token = tree.node_token(node_id);

llvm::errs() << "NodeId(kind: ";
kind.Print(llvm::errs());
llvm::errs() << ", token: ";
Lex::DumpIdImpl(tree.tokens(), token);
llvm::errs() << ")";
}

// A set of DumpId() overloads that dump an object to stderr, useful for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Lex, this is moved to the header, should this comment move to the header here too? Or can we make it consistent either way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a more complete comment to each header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a mention in docs/

// calling inside a debugger.
LLVM_DUMP_METHOD auto DumpId(const Parse::Tree& tree, Lex::TokenIndex token)
-> void {
Lex::DumpId(tree.tokens(), token);
}

LLVM_DUMP_METHOD auto DumpId(const Parse::Tree& tree, Parse::NodeId node_id)
-> void {
DumpIdImpl(tree, node_id);
llvm::errs() << '\n';
}

} // namespace Carbon::Parse

#endif // NDEBUG
24 changes: 24 additions & 0 deletions toolchain/parse/dump_id.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef CARBON_TOOLCHAIN_PARSE_DUMP_ID_H_
#define CARBON_TOOLCHAIN_PARSE_DUMP_ID_H_

#ifndef NDEBUG

#include "toolchain/parse/tree.h"

namespace Carbon::Parse {

auto DumpIdImpl(const Tree& tree, NodeId node_id) -> void;

auto DumpId(const Tree& tree, Lex::TokenIndex token) -> void;

auto DumpId(const Tree& tree, NodeId node_id) -> void;

} // namespace Carbon::Parse

#endif // NDEBUG

#endif // CARBON_TOOLCHAIN_PARSE_DUMP_ID_H_
Loading