-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adding parallel processing of many files. #57
Conversation
Most probably due to arguments (the way it is defined and used) it is getting a bit out of hand currently. Will try to do the necessary changes to make it work. |
@jacob-carlborg Although I am unable to find exactly where |
Compiler compiler; | ||
bool helpFlag; | ||
Language language; | ||
string[] argsToRestore; |
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.
Please use spaces (four) for indentation.
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.
|
inputFiles ~= arguments.argument.input; | ||
startConversion(inputFiles.first); | ||
int j = 0; | ||
if(remainingArgs.length >0) |
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.
Always put a space before the opening parenthesis in statements.
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.
Sorry. Will do.
I'll look into dstack and try to figure out how to implement this. |
26e55e5
to
edf6779
Compare
@jacob-carlborg Please review. It is working fine for me now. |
Most things I commented on are not fixed as far as I can see. |
if (arguments.argument.input.hasValue) | ||
{ | ||
inputFiles ~= arguments.argument.input; | ||
startConversion(inputFiles.first); | ||
int j = 0; |
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.
I don't thinks this is used.
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.
Sorry, had used it in previous code, forgot to remove...
I'm wondering if it's better that the |
Sure seems better if |
I did the changes. Also, please tell me how much to keep the |
And one of the buildbots is passing and other one is failing. I saw the output but can't figure out what actually is the error that is shown. Please help. |
The error messages are not very good. But the issue is that DStep fails to produce a result in the case of |
} | ||
|
||
else | ||
startConversion(""); | ||
{ | ||
auto conversionTask = task!startParsingFile(language, argsToRestore, "", false, arguments, compiler, compilerArgs); |
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.
I don't think it's necessary to use a task for this condition.
Again one build is passing, other one is not. The tests are failing on clang 3.7 and the best part is I have clang 3.7 only... How do I run these feature tests on my machine? Or what could be the probable cause for this failure? |
One building is running on Linux and the other one on OS X. The one on OS X is running additional tests, converting Objective-C to D. It's one of those that is failing. You either need OS X to run those test or install GNUStep on Linux. I think you can run basically the same tests by running |
That's a good question. You can always use |
If you are on linux try to install GNUStep and comment
The unit tests seems to pass.
|
@jacob-carlborg I have rebased the PR. Please consider merging if it's acceptable now. |
|
Actually, a couple of minor things:
|
19c256a
to
b82d53f
Compare
Is it okay now?
|
With the "help text" i meant the help text at the bottom of Application.d that is printed when the |
b82d53f
to
27aa465
Compare
With the "help text" i meant the help
text at the bottom of Application.d that
is printed when the --help flag is given.
But it was good that you update the
readme as well.
I saw that after pushing the changes. Is it complete now? I changed some
text in the readme as well.
|
Looking good now, I'll wait with merging until the tests are complete, just to be sure. |
Thanks. |
Although, i would take a couple of days break for a few other things.
But, it would be great if you could point me to some other issue/feature
that I could work on next.
|
@genuinelucifer I think a natural extension to this PR would be to be able to pass a directory to DStep, which scans the directory for files to translate. |
scans the directory for files to translate.
Ok. So a '-i <directory>` to check for all the '.h' files and then
translate them?
Or without a new flag. Simply check if inout file name is indeed a
directory and if yes then translate all files in it. In this case if a file
also exists with same name then that could be given priority.
Which one would you prefer?
|
The second approach. It's not possible to have a directory and a file with the same name. |
Oops. I thought it might be possible in linux. Never tried.
|
An alternative approach could be to allow this: |
So, i should hard code the recursive search for both cases? Because i could |
Never mind the globbing, the shell already does that. |
Oh great. :)
|
But I still think it's convenient to be able to pass a whole directory to translate. That should work on Windows as well, if we ever get DStep to work on Windows. |
Hmm. Right. Currently we only translate header files right? No source
files.
|
Yes, only header files. |
I think we can assume |
Oh hmm. Great.
|
I noticed one thing that might be a problem with the current implementation of multiple files. If I have a file structure looking like this:
And run DStep like
Looks like all files are placed in the top level of the target directory. That will cause problems if |
Oh hmm. But isn't it that the user should avoid? Translating files with
same name at the same time?
I mean even if we copy the directory structure then too it could be
dificult (or undesired) in some situations.
Probably a new flag for this?
|
I think it's a valid use case when using globbing and what I suggested as a new feature today, to handle a whole directory as input argument.
I think it's undesired for a single input file. But for multiple input files, I'm not sure. Maybe a new flag for this. |
Currently it works fine if one file is passed. But if I pass more than one file then i get only
extern(C):
in the output.I'm trying to find the mistake.
I would be thankful for any suggestion(s) regarding approach and/or the coding style (since it is my first PR to this project, I might have missed many coding styles standard for this projects)...