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 support for testing with stdin #4819

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jan 17, 2025

In order to write language-server tests, we need some way to pass stdin input. This adds support for a split "// --- STDIN" which will be provided as a temp file for testing.

Note this does more stdin -> input_stream style renaming, this is just bugging me more since I know shadowing works but it can be subtle to read, particularly since I'm now making direct use of stdin in a handful of spots.

@github-actions github-actions bot added the explorer Action items related to Carbon explorer code label Jan 17, 2025
@jonmeow
Copy link
Contributor Author

jonmeow commented Jan 17, 2025

Based on #4812

Copy link
Contributor

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Looks OK - wouldn't've minded a few more independent changes (renames, some drive by fixes I think (case_insensitive "error:" find to plain find), etc)

return {{.success = true}};
}

// Does printing and returns expected results for stdin.carbon.
Copy link
Contributor

Choose a reason for hiding this comment

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

Had a minor hiccup reading this as starting a question ("Does printing <do this?>") rather than descriptive. Ambiguity might be removed by rephrasing to "Prints and returns ... "?

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

Comment on lines 25 to +28
// Standard output; stdout.
llvm::raw_pwrite_stream& output_stream;
llvm::raw_pwrite_stream* output_stream;
// Error output; stderr.
llvm::raw_pwrite_stream& error_stream;
llvm::raw_pwrite_stream* error_stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

How come these became pointers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style:

When storing an object's address as a non-owned member, prefer storing a pointer.

https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/cpp_style_guide.md#syntax-and-formatting

Note this is covered by #4812

Comment on lines 15 to 17
auto Run(std::FILE* input_stream, llvm::raw_ostream* output_stream,
llvm::raw_ostream* error_stream) -> ErrorOr<Success> {
CARBON_CHECK(input_stream && output_stream && error_stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

If input_stream and output_stream have to be non-null, could they be passed by reference here instead of by pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style:

If it is captured and must outlive the call expression itself, use a pointer and document that it must not be null (unless it is also optional).

https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/cpp_style_guide.md#syntax-and-formatting

Note this is covered by #4812

Copy link
Contributor Author

@jonmeow jonmeow Jan 17, 2025

Choose a reason for hiding this comment

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

Oh, wait I guess these can be references. I'll update #4812.

@jonmeow
Copy link
Contributor Author

jonmeow commented Jan 17, 2025

Looks OK - wouldn't've minded a few more independent changes (renames, some drive by fixes I think (case_insensitive "error:" find to plain find), etc)

Did you notice that this is based on #4812? It looks like you're talking about changes from that PR's, instead of the "Add support for testing with stdin" commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants