Opened 12 years ago
Closed 11 years ago
#1114 closed defect (fixed)
LLVM warning: Value stored to '*' is never read
Reported by: | Echelon9 | Owned by: | |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 11 |
Component: | Core engine | Keywords: | variables, dead store |
Cc: | Patch: |
Description
LLVM compiler warnings of the form -- Value stored to '*' is never read -- have been highlighted in the attached patch.
Each line marked is an assignment to a variable which is then never subsequently read. Some appear redundant, whereas others like ok = JS_CallFunctionValue() are valid function calls where there is no need to save the return value, if we aren't using it later in the codebase.
Any concerns against adjusting or removing each dead store variable accordingly?
Attachments (1)
Change History (10)
by , 12 years ago
Attachment: | t1114-llvm-dead-store.patch added |
---|
comment:1 by , 12 years ago
Replying to Echelon9:
Any concerns against adjusting or removing each dead store variable accordingly?
I'd agree if they were causing problems, like a noticeable impact on performance. Otherwise, they could easily be bugs where we should but don't do something with the value, and if we remove the assignments then the bug becomes less obvious.
follow-up: 5 comment:2 by , 12 years ago
Also, might an optimizing compiler catch these cases and just not perform the assignment in the first place?
comment:3 by , 12 years ago
Keywords: | review removed |
---|
comment:4 by , 12 years ago
Milestone: | Alpha 9 → Backlog |
---|---|
Priority: | Should Have → Nice to Have |
comment:5 by , 12 years ago
For the two JS_CallFunctionValue
, we don't need to report errors on returning false (since it'll already be obvious if they failed), so the "ok =
" should be removed.
For CInput.cpp
, I've never understood its logic and would be reluctant to change it even if it's clearly wrong (unless the change is to rewrite the whole thing to be much clearer).
For the other two files, the assignments look like obsolete code and should be removed.
Replying to historic_bruno:
Also, might an optimizing compiler catch these cases and just not perform the assignment in the first place?
Yes, which is how LLVM is discovering it should complain about them :-) . But performance is irrelevant for all this code anyway - the problem is mostly that redundant assignments make the intent of the code less clear, so it's good to clean them up.
comment:8 by , 12 years ago
Finally got around to that, I removed all of them except CInput.cpp
for the above reasons, and one had already been fixed. We should run that check again to see if there's any new cases and then close this ticket.
comment:9 by , 11 years ago
Milestone: | Backlog → Alpha 11 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Patch highlighting the lines LLVM reported