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

Fixed install warning #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed install warning #86

wants to merge 1 commit into from

Conversation

coderaiser
Copy link

./src/unix/pty.cc: In function ‘v8::Handle<v8::Value> PtyFork(const v8::Arguments&)’:
../src/unix/pty.cc:185:34: warning: ignoring return value of ‘int chdir(const char*)’, declared with attribute warn_unused_result [-Wunused-result]
       if (strlen(cwd)) chdir(cwd);

@JamesMGreene
Copy link

Isn't your if condition backward?

@coderaiser
Copy link
Author

ignoring return value of ‘int chdir(const char*)’, declared with attribute warn_unused_result

Looks like result of chdir should not be ignored and this if condition checks it.

@JamesMGreene
Copy link

Oh, I suppose chdir returns 1 for errors rather than false, huh? If so, my mistake.

@coderaiser
Copy link
Author

On success, zero is returned. On error, -1 is returned.
chdir(2).

@JamesMGreene
Copy link

Gotcha, sorry for the confusion!

@coderaiser
Copy link
Author

That's OK, I don't think that it would be merged sometime anyway, I created this pull request nearly year ago :). Nobody looks after this repository. I even tried to find something similar to pty.js and tty.js but under active development. Nothing unfortunately.

@JamesMGreene
Copy link

That's actually why I commented: I forked the repo and was going to start merging pull requests into my own version of it, so I wanted to verify the correctness of this change. 😄

@coderaiser
Copy link
Author

I see, I think it's great idea. I thin this commit good thing to merge, because of install warning. Anyway it is better to change perror("chdir failed"); -> perror("chdir " + cwd + " failed");

@JamesMGreene
Copy link

Noted, thanks!

JamesMGreene added a commit to JamesMGreene/node-partty that referenced this pull request Jun 22, 2015
JamesMGreene added a commit to JamesMGreene/node-partty that referenced this pull request Jun 22, 2015
@JamesMGreene
Copy link

@coderaiser:
If you're interested, you can check out my fork now:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants