Rewrite strip_whitespace and remove readline.c #3275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ascent12
approved these changes
Dec 9, 2018
emersion
reviewed
Dec 9, 2018
RyanDwyer
reviewed
Dec 9, 2018
common/stringop.c
Outdated
|
|
||
| if (!*str) return; | ||
|
|
||
| while (isspace(str[--len])) {} |
There was a problem hiding this comment.
Don't you need to subtract start from len before doing this? len is still the original length of the string.
emersion
reviewed
Dec 9, 2018
sway/commands.c
Outdated
| } | ||
| // Split command list | ||
| cmdlist = argsep(&head, ";"); | ||
| cmdlist += strspn(cmdlist, whitespace); | ||
| for (; isspace(*cmdlist); ++head) {} |
emersion
reviewed
Dec 9, 2018
sway/config.c
Outdated
| @@ -1,4 +1,5 @@ | |||
| #define _XOPEN_SOURCE 600 // for realpath | |||
| #define _POSIX_C_SOURCE 200809L | |||
| #define _XOPEN_SOURCE 500 // for realpath | |||
There was a problem hiding this comment.
Don't define both, they'll cause issues because _XOPEN_SOURCE defines implicitly _POSIX_C_SOURCE
There was a problem hiding this comment.
I did not know that, thanks
emersion
reviewed
Dec 9, 2018
emersion
reviewed
Dec 9, 2018
sway/main.c
Outdated
| size_t line_size = 0; | ||
| ssize_t nread; | ||
| while ((nread = getline(&line, &line_size, f)) != -1) { | ||
| if (line[nread -1] == '\n') { |
ianyfan
force-pushed
the
remove-readline
branch
from
be708de
to
b1e90e5
Compare
RedSoxFan
reviewed
Dec 11, 2018
ianyfan
changed the title
|
Ah, that's a good point. And since that functionality is needed in quite a few places, maybe this isn't a change worth doing. |
|
I'd still like to see readline.c go away |
ianyfan
force-pushed
the
remove-readline
branch
from
b1e90e5
to
b56edaf
Compare
ianyfan
changed the title
|
I've added a commit to address the backslash continuation issue, hopefully this works? It's not the prettiest though... |
RedSoxFan
approved these changes
Jan 1, 2019
All occurrences of read_line have been replaced by getline. peek_line has been absorbed into detect_brace.
ianyfan
force-pushed
the
remove-readline
branch
from
b56edaf
to
bd35879
Compare
emersion
reviewed
Jan 1, 2019
common/stringop.c
Outdated
| size_t start = strspn(str, whitespace); | ||
| memmove(str, &str[start], len + 1 - start); | ||
|
|
||
| if (!*str) return; |
|
Thanks! |
|
Oh, I was going to squash the commits, but forgot, sorry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
strip_whitespacewas pretty awfully written.read_linehas been replaced by getline, hopefully I've rewritten everything correctly.peek_linewas only used once, and the way it was being used was quite inefficient, so I just absorbed it intodetect_brace.