Issue 9287 - Implement -stdin.#6880
Conversation
|
Note that I had to hack together a stdio-based way of copying data from stdin into a buffer, because the existing |
quickfur
force-pushed
the
stdin
branch
3 times, most recently
from
85d13f5 to
34d8d78
Compare
src/ddmd/mars.d
Outdated
| size_t pos = 0; | ||
| size_t sz = bufIncrement; | ||
|
|
||
| ubyte* buf = cast(ubyte*).malloc(sz + 1); // +1 for sentinel |
There was a problem hiding this comment.
malloc is only meant for buffers which are freed later.
use xmalloc amd xrealloc instead
even better would be in walking the length of the input and the using allocmemory.
There was a problem hiding this comment.
Went with xmalloc / xrealloc. Didn't really want to walk the input, since it could be large, if xrealloc already takes care of allocating a large-enough buffer for us.
|
@quickfur what is the specific usecase for this ? |
|
@quickfur if you use "Fix " as a commit message, the bot will recognize it (more infos). @UplinkCoder most tools/compilers allow this - this is nice for a bunch of use cases starting with showing compile errors in my favorite editor (most editors read the un-saved instance in-memory and thus can nicely pipe it to |
|
Also, please advise if there is a better place to put |
|
Protocol: when creating a PR to fix Issue https://issues.dlang.org/show_bug.cgi?id=9287, add a link in bugzilla to the PR. I already did so for this. |
|
As I commented in https://issues.dlang.org/show_bug.cgi?id=9287 please use |
Sorry to bother you with this, but adding a test in the testsuite for this will highly increase chances of acceptance ;-) |
|
@WalterBright I did put the PR link in bugzilla, did you miss it? As for '-' instead of '-stdin', I'll get to it later. Thanks for the feedback. PS: where's the best place to put 'copyFile'? |
|
@wilzbach How would you test something involving standard input? I don't know if the current test suite runner is capable of piping code via stdin. |
The test suite is Turing-complete (it allows the use of Bash), see e.g. this simple test: #!/usr/bin/env bash
src=runnable${SEP}extra-files
dir=${RESULTS_DIR}${SEP}runnable
output_file=${dir}/test10567.sh.out
$DMD -m${MODEL} -I${src} -of${dir}${SEP}test10567a${OBJ} -c ${src}${SEP}test10567a.d || exit 1
$DMD -m${MODEL} -I${src} -of${dir}${SEP}test10567${EXE} ${src}${SEP}test10567.d ${dir}${SEP}test10567a${OBJ} || exit 1
${RESULTS_DIR}/runnable/test10567${EXE} || exit 1
rm ${dir}/{test10567a${OBJ},test10567${EXE}}
echo Success >${output_file}Simply rewriting sth. like this to use |
|
Thanks for the tip! Got a working test now. |
|
Hmm, the Jenkins errors appear unrelated to the changes in this PR. What's going on? |
|
Nevermind, it seems to be affecting other PRs too. |
src/ddmd/mars.d
Outdated
| size_t sz = bufIncrement; | ||
|
|
||
| ubyte* buf = cast(ubyte*)mem.xmalloc(sz + 1); // +1 for sentinel | ||
| if (buf is null) |
There was a problem hiding this comment.
xmalloc() never returns null
src/ddmd/mars.d
Outdated
| assert(sz > pos); | ||
| size_t rlen; | ||
| pos += rlen = fread(buf + pos, 1, sz - pos, fp); | ||
| while (!ferror(fp) && !feof(fp)) |
There was a problem hiding this comment.
This loop can be written so there is only one xmalloc call instead of two, and one fread instead of two.
src/ddmd/mars.d
Outdated
| { | ||
| if (pos == sz) | ||
| { | ||
| sz += bufIncrement; |
There was a problem hiding this comment.
Reallocating every 4K bytes makes for a very slow read. expression.d is 500K, so that makes for 125 copies and reallocations. I'd probably make it at least 128K.
src/ddmd/mars.d
Outdated
| assert(sz > pos); | ||
| pos += rlen = fread(buf + pos, 1, sz - pos, fp); | ||
| } | ||
| if (ferror(fp)) |
src/ddmd/mars.d
Outdated
| /** | ||
| * Copies data from a FILE* into a newly-allocated buffer. | ||
| * | ||
| * The buffer is always null-terminated. |
There was a problem hiding this comment.
Use Ddoc style comments, with Params: and Returns: sections.
src/ddmd/mars.d
Outdated
| } | ||
| if (global.params.readStdin) | ||
| { | ||
| files.push(cast(char*)global.stdin_d); // a dummy name, we don't actually look it up |
There was a problem hiding this comment.
maybe put this before line 1134, then the test doesn't have to be done twice
src/ddmd/mars.d
Outdated
| } | ||
| } | ||
| } | ||
| if (global.params.readStdin) |
There was a problem hiding this comment.
Seems like this code should go into root/file.d, as reading from stdin should be a low level thing, not a high level one.
|
I suggest that if |
|
great review (and idea) @WalterBright |
|
Ah, he's expecting the word "issue". Got it, thanks! |
Exact regex-> https://github.com/dlang-bots/dlang-bot#nerdy-details |
|
ping @WalterBright @andralex Is this PR satisfactory, or should a different approach be taken? Please advise, thanks! |
|
It seems the PR cannot be made much simpler. I'm mildly in favor of the notion of accepting piped input, without being able to think of a solid application. OK with me. @WalterBright ? |
src/ddmd/mars.d
Outdated
| } | ||
| } | ||
| else if (p[1] == '\0') | ||
| files.push("__stdin"); |
There was a problem hiding this comment.
Why not pushing __stdin.d to avoid ambiguities?
|
Rebased, hopefully this fixes the travis error which seems unrelated. @andralex What else needs to be done to get this PR merged? |
|
@WalterBright 's call |
|
@andralex I was referring to your change requested review. @WalterBright already approved this PR. |
|
So why do we should we add verbose changelog entries?
Of course, it's in the changelog, but at the end and rather hard to spot:
|
|
Heh, I implemented this feature two months before this PR, and it seems to have the same |
10 participants
It's nice to be able to pipe source code into dmd via standard input, e.g., in code generation programs so that you don't have to create a temporary file just to be able to compile the code.
Based on the bug notes in the original bug, using
-to mean stdin is probably a problematic idea, so I have chosen a different route, i.e., use-stdinto indicate that dmd should read source code from stdin. I think this is clearer, and less *nix-specific.