This is an archive of the discontinued LLVM Phabricator instance.

Avoid use of gets() in DOE-ProxyApps-C/Pathfinder
ClosedPublic

Authored by arichardson on Oct 16 2020, 8:43 AM.

Details

Summary

gets() has been removed from C11 and on FreeBSD 13 libc no longer provides
it by default. Use fgets() instead.

Event Timeline

arichardson created this revision.Oct 16 2020, 8:43 AM
arichardson requested review of this revision.Oct 16 2020, 8:43 AM
jrtc27 requested changes to this revision.Oct 17 2020, 7:17 AM
jrtc27 added a subscriber: jrtc27.
jrtc27 added inline comments.
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

gets always removes the newline, but fgets will include it (except in the case where the input reaches EOF before one is found) so the final character needs to be removed if it's \n.

This revision now requires changes to proceed.Oct 17 2020, 7:17 AM

remove newline

arichardson marked an inline comment as done.Oct 18 2020, 10:51 AM
krytarowski added inline comments.Oct 20 2020, 2:57 PM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

Can you introduce mygets that implements gets_s internally?

arichardson added inline comments.Oct 21 2020, 6:39 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

I could do, but that seems like overkill for the one use of gets().

krytarowski added inline comments.Oct 21 2020, 6:44 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302
static char *mygets(char *str, size_t n)
{
    char *ret;
    size_t last;

    if ((ret = fgets(str, n, stdin)) != NULL) {
        last = strlen(str) - 1;

        if (str[last] == '\n' || str[last] == '\r' )
            str[last] = '\0';
    }

    return ret;
}

And then use mygets(stringBuffer, sizeof(stringBuffer));

It looks nicer to me.

arichardson added inline comments.Oct 21 2020, 7:07 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

That will not work with zero-length strings (it will access str[-1]),

Adding stringBuffer[strcspn(stringBuffer, "\r\n")] = '\0'; seems much simpler to me.

jrtc27 added inline comments.Oct 21 2020, 7:10 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

Also the \r check is misguided. In text mode, line endings get normalised to just \n. In binary mode they will stay as-is, but on Windows that means you have \r\n (and \r on classic Mac OS 9 and below I guess) so the if would need to be a while and last decremented. But stdin should be in text mode by default.

arichardson added inline comments.Oct 21 2020, 7:12 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

strcspn() returns the length up to the first occurrence of \r or \n, so it should work?

jrtc27 added inline comments.Oct 21 2020, 7:14 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

Yeah, that avoids any such complications. Technically the \r can just be dropped from it I believe but it does no harm (well unless people feed in \r on systems with Unix line endings I guess...).

krytarowski added inline comments.Oct 21 2020, 8:20 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

That will not work with zero-length strings (it will access str[-1]),

How so? fgets returns NULL for 0-length strings.

I find this switch more elegant with gets_s()-like code, but this is a matter of taste.

arichardson added inline comments.Oct 21 2020, 8:26 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

I'd prefer gets_s, but we can't assume that the platform provides it. using fgets+ strcspn is only one additional line and supported everywhere so it's IMO the simpliest solution. If there were more uses of gets(), then a wrapper might make sense.

krytarowski added inline comments.Oct 21 2020, 10:47 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302

I'd prefer gets_s, but we can't assume that the platform provides it.

As I proposed, including it inline as mygets or similar, as almost no platforms delivers it.

using fgets+ strcspn is only one additional line and supported everywhere so it's IMO the simpliest solution.

In my personal taste open-coding this looks awful, but this is up to you.

If the LOC would matter in my proposal, mygets could be compressed to 2 lines too.

If there were more uses of gets(), then a wrapper might make sense.

This is fine.

dim updated this revision to Diff 364981.Aug 7 2021, 1:37 PM

Add mygets() and implement it hopefully to everybody's liking.

jrtc27 added inline comments.Aug 7 2021, 1:40 PM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
290
  1. len >= 1, no? Otherwise a blank line will not have a newline stripped.
  2. This doesn't handle \r\n correctly; either assume stdin is in text mode so you only ever see \n or find the first instance of one of the characters (either with strcspn or by a loop that walks backwards here)
dim updated this revision to Diff 364982.Aug 7 2021, 1:43 PM

Assume stdin is in text mode and compare correctly.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 9 2021, 1:17 AM
This revision was automatically updated to reflect the committed changes.