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

adds support for optional args, with backwards compatibility #64

Merged
merged 12 commits into from
May 15, 2017

Conversation

dmalan
Copy link
Member

@dmalan dmalan commented Apr 9, 2017

This changes get_* to expect a string as their sole parameter, which is then used to prompt (and, if need be, re-prompt) the user for the intended type of input. Our prior use of a Retry: prompt was feeling arbitrary. Plus, this way, students can control not only the first prompt a user sees but any (identical) re-prompts.

Maintains backwards compatibility with prior versions of the library such that students (for now) can still call Get* and get_* with no arguments; in those cases, the library behaves as it used to with Retry: re-prompts.

After CS50 AP 1617 wraps in June 2017, we can remove support for Get* altogether (or just leave the deprecation warnings in there for a year). After CS50 AP 1718 wraps in June 2018, we can remove support for optional arguments to get_*, instead requiring that a user provide a prompt (which could still be ""); probably no need at that point to allow NULL as input, since that's only used now as a sentinel value to indicate that a caller is using the functions' older argument-less calling conventions.

@dmalan
Copy link
Member Author

dmalan commented Apr 9, 2017

Per #50, not yet clear how best to handle the std=c99 issue, if not via std=gnu99.

@dmalan
Copy link
Member Author

dmalan commented Apr 13, 2017

Added macro checks for GNU extensions.

@dmalan
Copy link
Member Author

dmalan commented Apr 13, 2017

@kzidane, @crossroads1112 able to test on Visual C?

@dmalan
Copy link
Member Author

dmalan commented Apr 29, 2017

:shipit:

@cmlsharp
Copy link
Contributor

cmlsharp commented Apr 30, 2017

Sorry for my being late, but as an alternative, what about temporarily (until optional arguments are no longer required) doing something like the following:

In cs50.h:

struct OptionalPrompt {
    char _sentinel; // Avoid __VA_ARGS__ issues
    string prompt;
}

char get_char_prompt(struct OptionalPrompt *opt_prompt);
#define get_char(...) get_char_prompt(&(struct OptionalPrompt) {._sentinel = 0, __VA_ARGS__})

In cs50.c:

char get_char_prompt(struct OptionalPrompt *opt_prompt) 
{
    string prompt = opt_prompt->prompt;
    // The rest of get_char's implementation
}

In main.c:

int main(void) 
{
    /* expands to get_char_prompt(&(struct OptionalPrompt) { .sentinel = 0, }) which is completely valid
     * opt_arg->prompt is implicitly set to 0 (NULL) since it isn't specified
    */
    char c1 = get_char(); 

    /* expands to get_char_prompt(&(struct OptionalPrompt) { .sentinel = 0, "Enter a character" }) */
    char c2 = get_char("Enter a character: ");
}

Pros: Completely standard ISO C (no need to worry about VC vs GNUC or change the Makefile)
Cons: It does mean that the functions in cs50.c will not actually be named get_char but rather some similar name like get_char_prompt. This does add some complexity but I'd argue significantly less than the preprocessor solution currently in cs50.h.

@dmalan
Copy link
Member Author

dmalan commented May 15, 2017

@crossroads1112, I worry that introducing the *_prompt variants is wont to cause confusion (or at least curiosity) during debugging, when the symbols might appear among stack frames. Better, I think, to hide the backwards compatibility with some cleverness for just a year, especially since we're relying on extensions for garbage collection anyway.

If this proves an issue for, e.g., Windows, we can just offer a version 9.0.0 branch in parallel with support only for the required-argument functions.

@dmalan dmalan merged commit a1e51fc into master May 15, 2017
@dmalan dmalan deleted the optional-args branch May 15, 2017 17:27
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.

None yet

3 participants