Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
I like the change. All of my comments below describe existing issues in the code, nothing you introduced in this change. Maybe they can be addressed separately.
| contrib/elftoolchain/strings/strings.c | ||
|---|---|---|
| 249–251 ↗ | (On Diff #50561) | I think this check can be removed. |
| 254 ↗ | (On Diff #50561) | This memset is unnecessary |
| 298 ↗ | (On Diff #50561) | This return type is bogus on 32-bit arch (sizeof(long) == 4). EOF (-1) is indistinguishable from a valid 32-bit value. |
| 306–310 ↗ | (On Diff #50561) | Isn't this bogus? It will spuriously fail to read the last byte at the end of the file. It also mishandles stream errors prior to EOF. I suggest: c = getc(pfile);
if (c == EOF)
return (EOF);instead. |
| 325–330 ↗ | (On Diff #50561) | This math is undefined behavior on 32-bit long systems, I think (left shift into sign bit). Probably the "value" result should be a 32-bit unsigned integer. And the return type can either be a wider signed type (int64_t) or the error status can be separated from the return value (i.e., uint32_t *value output parameter). |
| 365 ↗ | (On Diff #50561) | I don't think there's any reason to keep this conditional on feof(pfile). EOF could also indicate an IO error rather than actual end of file. We don't want to continue if getcharacter() returned EOF regardless of anything else. |
| 412–417 ↗ | (On Diff #50561) | EOF check should be done before (uint8_t)c > 127 test, which for c == EOF will always succeed. |
I like this change.
Adding @kaiw and @jkoshy_users.sourceforge.net to CC as a heads-up as we will want this to go upstream as well. (I can take care of the upstream commit.)
| contrib/elftoolchain/strings/strings.c | ||
|---|---|---|
| 249–251 ↗ | (On Diff #50561) | Thanks. You are right I will address that. |
| 254 ↗ | (On Diff #50561) | Thanks. You are right I will address that. |
| 298 ↗ | (On Diff #50561) | Thanks. You are right I will fix that as separate commit. |
| 306–310 ↗ | (On Diff #50561) | Thanks. You are right I will fix that as separate commit. |
| 325–330 ↗ | (On Diff #50561) | Thanks. You are right I will fix that as separate commit. |
| 365 ↗ | (On Diff #50561) | Thanks. You are right I will fix that as separate commit. |
| 412–417 ↗ | (On Diff #50561) | Thanks. You are right I will fix that as separate commit. |