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.

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

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

Merged
merged 22 commits into from
Dec 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
b5d253a
Add DumpId() methods to Check::Context, Parse::Tree, Lex::TokenizedBu…
danakj Dec 3, 2024
9cc9ac4
add-missing-files
danakj Dec 4, 2024
e28d533
parse-tree
danakj Dec 4, 2024
30bb063
iwyu
danakj Dec 4, 2024
19fddbc
dump import locations
danakj Dec 4, 2024
08907b1
dump-methods-in-base-classes
danakj Dec 4, 2024
1526f1d
add-parse-and-overload-of-lex
danakj Dec 5, 2024
bdc9c01
use-if-constexpr
danakj Dec 5, 2024
b0b1cdf
simplify
danakj Dec 5, 2024
91bc8a0
simply
danakj Dec 5, 2024
506a504
add-dump-content
danakj Dec 5, 2024
fb7840f
review-feedback
danakj Dec 9, 2024
228273f
write-newline
danakj Dec 9, 2024
a8da68b
Merge branch 'trunk' into dump
danakj Dec 9, 2024
3ed5149
dont-avoid-include-ostream
danakj Dec 10, 2024
2bec55b
remove-base-class
danakj Dec 10, 2024
d5a2374
Merge remote-tracking branch 'refs/remotes/origin/trunk' into dump
danakj Dec 10, 2024
1964585
Switch to free functions
jonmeow Dec 11, 2024
4fa1b64
Review cleanup
jonmeow Dec 12, 2024
c9d28da
prototypes
jonmeow Dec 12, 2024
3365c6c
doc
jonmeow Dec 12, 2024
baa3dda
Address comments
jonmeow Dec 12, 2024
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
21 changes: 19 additions & 2 deletions toolchain/check/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ cc_library(
"convert.cpp",
"decl_name_stack.cpp",
"deduce.cpp",
"dump_id.cpp",
"eval.cpp",
"function.cpp",
"generic.cpp",
Expand All @@ -47,7 +46,6 @@ cc_library(
"decl_name_stack.h",
"deduce.h",
"diagnostic_helpers.h",
"dump_id.h",
"eval.h",
"function.h",
"generic.h",
Expand Down Expand Up @@ -93,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 @@ -112,6 +128,7 @@ cc_library(
hdrs = ["check.h"],
deps = [
":context",
":dump_id",
":impl",
":interface",
":pointer_dereference",
Expand Down
14 changes: 0 additions & 14 deletions toolchain/check/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "toolchain/check/decl_introducer_state.h"
#include "toolchain/check/decl_name_stack.h"
#include "toolchain/check/diagnostic_helpers.h"
#include "toolchain/check/dump_id.h"
#include "toolchain/check/generic_region_stack.h"
#include "toolchain/check/global_init.h"
#include "toolchain/check/inst_block_stack.h"
Expand Down Expand Up @@ -611,19 +610,6 @@ class Context {
return bind_name_cache_;
}

// A set of DumpId() overloads that dump an object to stderr, useful for
// calling inside a debugger.
LLVM_DUMP_METHOD auto DumpId(Lex::TokenIndex token) const -> void {
tokens().DumpId(token);
}
LLVM_DUMP_METHOD auto DumpId(Parse::NodeId node_id) const -> void {
parse_tree().DumpId(node_id);
}
LLVM_DUMP_METHOD auto DumpId(SemIR::LocId loc_id) const -> void {
DumpIdImpl(*this, loc_id);
llvm::errs() << '\n';
}

private:
// A FoldingSet node for a type.
class TypeNode : public llvm::FastFoldingSetNode {
Expand Down
25 changes: 22 additions & 3 deletions toolchain/check/dump_id.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,21 @@
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include "toolchain/check/dump_id.h"
#ifndef NDEBUG

#include "toolchain/lex/dump_id.h"

#include "common/check.h"
#include "common/ostream.h"
#include "toolchain/check/context.h"
#include "toolchain/lex/dump_id.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 {

auto DumpIdImpl(const Context& context, SemIR::LocId loc_id) -> void {
static auto DumpIdImpl(const Context& context, SemIR::LocId loc_id) -> void {
if (!loc_id.is_valid()) {
llvm::errs() << "LocId(invalid)";
return;
Expand Down Expand Up @@ -44,4 +45,22 @@ auto DumpIdImpl(const Context& context, SemIR::LocId loc_id) -> void {
}
}

// 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 •
edited
Loading

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 •
edited
Loading

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
18 changes: 0 additions & 18 deletions toolchain/check/dump_id.h
View file
Open in desktop

This file was deleted.

22 changes: 14 additions & 8 deletions toolchain/lex/BUILD
View file
Open in desktop
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 All @@ -209,14 +221,8 @@ cc_library(

cc_library(
name = "tokenized_buffer",
srcs = [
"dump_id.cpp",
"tokenized_buffer.cpp",
],
hdrs = [
"dump_id.h",
"tokenized_buffer.h",
],
srcs = ["tokenized_buffer.cpp"],
hdrs = ["tokenized_buffer.h"],
deps = [
":character_set",
":helpers",
Expand Down
21 changes: 15 additions & 6 deletions toolchain/lex/dump_id.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,37 @@
// 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"
#include "toolchain/lex/tokenized_buffer.h"

namespace Carbon::Lex {

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

auto kind = buffer.GetKind(token);
auto line = buffer.GetLineNumber(token);
auto col = buffer.GetColumnNumber(token);
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(buffer.source().filename());
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
12 changes: 10 additions & 2 deletions toolchain/lex/dump_id.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,22 @@
#ifndef CARBON_TOOLCHAIN_LEX_DUMP_ID_H_
#define CARBON_TOOLCHAIN_LEX_DUMP_ID_H_

#include "toolchain/lex/token_index.h"
#ifndef NDEBUG

#include "toolchain/lex/tokenized_buffer.h"

namespace Carbon::Lex {

class TokenizedBuffer;

auto DumpIdImpl(const TokenizedBuffer& buffer, TokenIndex token) -> void;
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: 0 additions & 8 deletions toolchain/lex/tokenized_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "toolchain/base/mem_usage.h"
#include "toolchain/base/shared_value_stores.h"
#include "toolchain/diagnostics/diagnostic_emitter.h"
#include "toolchain/lex/dump_id.h"
#include "toolchain/lex/token_index.h"
#include "toolchain/lex/token_kind.h"
#include "toolchain/source/source_buffer.h"
Expand Down Expand Up @@ -217,13 +216,6 @@ class TokenizedBuffer : public Printable<TokenizedBuffer> {

auto source() const -> const SourceBuffer& { return *source_; }

// A set of DumpId() overloads that dump an object to stderr, useful for
// calling inside a debugger.
LLVM_DUMP_METHOD auto DumpId(Lex::TokenIndex token) const -> void {
DumpIdImpl(*this, token);
llvm::errs() << '\n';
}

private:
friend class Lexer;
friend class TokenDiagnosticConverter;
Expand Down
15 changes: 13 additions & 2 deletions toolchain/parse/BUILD
View file
Open in desktop
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 All @@ -113,13 +126,11 @@ cc_library(
cc_library(
name = "tree",
srcs = [
"dump_id.cpp",
"extract.cpp",
"tree.cpp",
"tree_and_subtrees.cpp",
],
hdrs = [
"dump_id.h",
"tree.h",
"tree_and_subtrees.h",
],
Expand Down
18 changes: 17 additions & 1 deletion toolchain/parse/dump_id.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@
// 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"
#include "toolchain/parse/tree.h"

namespace Carbon::Parse {

Expand All @@ -26,4 +27,19 @@ auto DumpIdImpl(const Tree& tree, NodeId node_id) -> void {
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
12 changes: 9 additions & 3 deletions toolchain/parse/dump_id.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,20 @@
#ifndef CARBON_TOOLCHAIN_PARSE_DUMP_ID_H_
#define CARBON_TOOLCHAIN_PARSE_DUMP_ID_H_

#include "toolchain/parse/node_ids.h"
#ifndef NDEBUG

namespace Carbon::Parse {
#include "toolchain/parse/tree.h"

class Tree;
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
Loading