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

IO module clean ups #14395

Closed
1 of 5 tasks
lydia-duncan opened this issue Nov 5, 2019 · 7 comments · Fixed by #15092
Closed
1 of 5 tasks

IO module clean ups #14395

lydia-duncan opened this issue Nov 5, 2019 · 7 comments · Fixed by #15092

Comments

@lydia-duncan
Copy link
Member

lydia-duncan commented Nov 5, 2019

Noticed while adding deprecation warnings when the IO module symbols are accessed without and explicit use of the module (as part of #14145)

  • I think the description of stringStyleExactLen doesn't match the behavior?
    • The other functions around it with a similar description for their return type actually return an iostringstyle, while this returns an int(64)
  • I think the description of stringStyleWithVariableLength doesn't match the behavior?
    • The other functions around it with a similar description for their return type actually return an iostringstyle, while this returns an int(64)
  • we should remove the deprecated version of open, which was deprecated 9 months ago
  • we don't document QioPluginFile or openplugin, but we don't "no doc" openplugin
  • we don't document stdinInit, stdoutInit, and stderrInit, but we also don't "no doc" them either. Should they be documented or "no doc"ed?
@lydia-duncan
Copy link
Member Author

There may be some other things that I will encounter along the way. Will address this issue once I am confident there won't be additional ones (or at least that I won't be looking for more)

@immadisairaj
Copy link
Member

I would like to work on this issue. These all are related to documentation right.
What does the stringStyleExactLen and stringStyleWithVariableLength do? I found them return the same number(input to that method) and -10.

@lydia-duncan
Copy link
Member Author

For stringStyleExactLen and stringStyleWithVariableLength, I don't know whether it is the documentation or the behavior that needs to change - it seems entirely possible that the documentation was right but the implementation was wrong, but it could be the other way around. I would ask either in gitter or the chapel-developers mailing list (but you'll want to ask about all of these regardless)

@immadisairaj
Copy link
Member

From the discussion what happened

  • "no doc" stringStyleExactLen, stringStyleWithVariableLength, stdinInit, stdoutInit, stderrInit.
  • document openplugin

Tasks to do along with the above mentioned

  • remove the depricated open method

@lydia-duncan I do have a doubt
do we have to remove stdinInit and other methods from deprication in LastResortIO.chpl and the corresponding test files?

@lydia-duncan
Copy link
Member Author

remove the depricated open method

This should already have occurred, that's why the box is checked

do we have to remove stdinInit and other methods from deprication in LastResortIO.chpl and the corresponding test files?

I don't think there's any harm in leaving them until we decide to no longer provide the deprecation warnings, but I also wouldn't object to removing them either - I'd lean towards just leaving them, personally. I think the likelihood of them being used by a user is very low, but if someone is using them, that person might as well get a warning about the functions not being included by default

@immadisairaj
Copy link
Member

This should already have occurred, that's why the box is checked

But I found this method in the module

pragma "no doc"
proc open(out error:syserr, path:string="",
          mode:iomode, hints:iohints=IOHINT_NONE,
          style:iostyle = defaultIOStyle()):file {
  compilerWarning("This version of open() is deprecated; " +
                  "please switch to a throwing version");
  error = ENOERR;
  var ret: file;
  try {
    ret = open(path, mode, hints, style);
  } catch e: SystemError {
    error = e.err;
  } catch {
    error = EINVAL;
  }
  return ret;
}

I'd lean towards just leaving them, personally

Even I prefer it leaving like that.

@lydia-duncan
Copy link
Member Author

Ah, that's a different version of open than I was thinking of. I think that one can be safely removed, sure!

lydia-duncan added a commit that referenced this issue Mar 9, 2020
add no doc to non-user io methods
[contributed by @immadisairaj, reviewed by Lydia]

Fixes: #14395 

- "no doc" `stringStyleExactLen`, `stringStyleWithVariableLength`, `stdinInit`, `stdoutInit`, `stderrInit`, `openplugin` in IO module
- remove deprecated `open` method

Passed full paratest with futures, and checked built docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants