Fixup filenames in comments; remove dead code found with scan-build. #1317

Merged
merged 2 commits into from Feb 27, 2014

Conversation

Projects
None yet
5 participants
Contributor

geoff-codes commented Feb 27, 2014

Sorry, this probably should be two different PRs...

The .c -> .cpp stuff should be self-explanatory.

The rest is removal of dead code found with the clang static analyzer. It definitely is, presently, all dead code - I can see no functionality change, and all tests pass - but whether its supposed to be doing something, I don't know. So you may wish to look over those.

~G

xfix pushed a commit that referenced this pull request Feb 27, 2014

Merge pull request #1317 from pullreq/cpp
Fixup filenames in comments; remove dead code found with scan-build.

@xfix xfix merged commit 74135c0 into fish-shell:master Feb 27, 2014

1 check passed

default The Travis CI build passed
Details

@geoff-codes geoff-codes deleted the pullreq:cpp branch Feb 27, 2014

Member

zanchey commented on 18dd6f5 Feb 28, 2014

What is share/doxygen_sqlite3.db and do we need it?

Member

xfix replied Feb 28, 2014

@zanchey: It's a temponary file I somehow missed. I think it would be a good idea to add it to .gitignore.

Contributor

terlar commented Feb 28, 2014

After this merge I cannot open an interactive terminal. It just freezes indefinetily.

Everything works fine with: version 2.1.0-395-g6d74978

Owner

ridiculousfish commented Feb 28, 2014

Reproduces for me. Bisects to ddcd2b0

Owner

ridiculousfish commented Feb 28, 2014

There are serious problems with that commit. Reverting it.

It's great to run the clang static analyzer, and I do so often. I wonder if there was some confusion about the meaning of "dead stores." A dead store is a write to a variable that is not later read. It is different from dead code, which is unreachable. Dead code can always be removed, but a dead store cannot be, if there's a side effect. Example:

ignore = write(2, mess, strlen(mess));

This may be a dead store, if ignore is not later read, but the write call obviously has side effects. As of this writing, I believe that every dead store is deliberate, designed to improve code readability.

clang also complains about mktemp, but its suggestion to switch to mkstemp is wrong, and there's a comment above the line in question explaining the rationale.

Owner

ridiculousfish commented Feb 28, 2014

Also reverting the prior commit 18dd6f5 due to the doxygen_sqlite3.db addition. I'll be happy to merge one without that file addition - the .c -> .cpp change is otherwise valuable.

ridiculousfish added a commit that referenced this pull request Feb 28, 2014

Revert "Merge pull request #1317 from pullreq/cpp"
This reverts commit 74135c0, reversing
changes made to 6d74978.

See discussion in #1317
Contributor

geoff-codes commented Feb 28, 2014

Ack! So sorry. I'm learning.

I wonder if there was some confusion about the meaning of "dead stores."

Yep.

I did try to say it probably needs to be reviewed, (but not really an excuse ...mea culpa).

floam added a commit that referenced this pull request Jun 12, 2016

Improve comments, update Doxyfile
Some changes were cribbed from #1317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment