Skip to content

Fixed install warning#86

Open
coderaiser wants to merge 1 commit into
chjj:masterfrom
coderaiser:install-warning
Open

Fixed install warning#86
coderaiser wants to merge 1 commit into
chjj:masterfrom
coderaiser:install-warning

Conversation

@coderaiser

Copy link
Copy Markdown
./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
Copy Markdown

Isn't your if condition backward?

@coderaiser

Copy link
Copy Markdown
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
Copy Markdown

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

@coderaiser

Copy link
Copy Markdown
Author

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

@JamesMGreene

Copy link
Copy Markdown

Gotcha, sorry for the confusion!

@coderaiser

Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

@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