-
Notifications
You must be signed in to change notification settings - Fork 248
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
Feat: Add ability to compile from stdin #1327
Conversation
source/cppfront.cpp
Outdated
@@ -84,7 +84,12 @@ auto main( | |||
} | |||
|
|||
// Load + lex + parse + sema | |||
cppfront c(arg.text); | |||
cppfront c = [&]() -> cppfront { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be less change to handle this split in the load()
function instead? https://github.com/hsutter/cppfront/blob/main/source/io.h#L885
It would need a little change where it currently checks for the file extension, but the rest may just work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea you're right. I was struggling a bit to figure out where to split this but I think that would make a lot more sense. Ill go ahead and give that a try. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that suggestion, that greatly simplified the code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome.
source/io.h
Outdated
if (!in.is_open()) { | ||
// If filename is stdin, we read from stdin, otherwise we try to read the file | ||
// | ||
std::ifstream fss{ filename }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a little more explicit, only compare once, and don't try to open stdin
.
auto is_stdin = filename == "stdin";
std::ifstream fss;
if (!is_stdin) {
fss.open(filename);
if (!fss.is_open()) {
return false;
}
}
std::istream& in = is_stdin ? std::cin : fss;
auto in_comment = false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: other than the "don't try to open stdin" because it will fail in unexpected ways if there is a file named stdin
in the working directory, the rest is personal preference, so just suggestions, feel free to do it differently.
Then again, maybe it would be expected to read that file, and stdin should be an explicit option rather than using the string stdin
in the filename place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, I missed that. Just pushed a fix.
Then again, maybe it would be expected to read that file, and stdin should be an explicit option rather than using the string
stdin
in the filename place.
Yea, I was thinking about this too. When I originally attempted this, I created a cli flag which would turn on stdin
mode. But I realized cppfront is using this similar logic with the -o
flag; if you specify -o stdout
, it will output to stdout instead of file, so I figured it was following convention to use the same logic for file reading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to be consistent for now.
Thanks for the reviews! |
Thanks for your pull request! It looks like this may be your first contribution to cppfront. I've emailed you the Contributor License Agreement (CLA), and once it's completed I can look at your pull request. Thanks again for your contribution. |
Thanks Herb! (I had read the in the section about contributing that we needed to sign something, and was wondering where we were supposed to sign, happy to see it just comes through email haha). I've signed and sent back the CLA |
Also: - re-ran self-build and regression tests - suppressed non-error output when stdout is used, so piping is easier
source/io.h
Outdated
// | ||
auto is_stdin = filename == "stdin"; | ||
std::ifstream fss; | ||
if (is_stdin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
missing? Otherwise compiling a named file breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. That was definitely my bad 😅 . Thanks for fixing
@@ -1179,6 +1179,7 @@ class cppfront | |||
if ( | |||
!sourcefile.ends_with(".cpp2") | |||
&& !sourcefile.ends_with(".h2") | |||
&& sourcefile != "stdin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is fine, but we need to also update later logic that generates the output filename and currently assumes it ends in one of these and so just truncates the last character (e.g., .cpp2
becomes .cpp
. When I tried to use stdin
the output went to a file named stdi
(last character blindly truncated).
How is this intended to work -- if you use stdin
, is the output expected to go to stdout
, or if to a named file what is the intended name?
I think the right answer is probably to default to stdout
unless the programmer overrode that with -o
. I'll implement that, and also make sure the output to stdout
is clean of other messages when there are no errors.
Thanks! |
This PR adds the ability to compile from
stdin
.This is useful so that cppfront can compile source code that's been piped in from another program.
For my use-case specifically: when working on the LSP server, we already have access to source code within a
TextDocument
variable.Prior to this PR, we have to either save the edited content to a temporary file, or save the source file itself prior to invoking cppfront, otherwise we end up with stale diagnostics data.
Happy to fix any issues anyone finds!
Thanks!