Remove null pointer checks when the pointer had already been used. Th… #7852
Conversation
…e code would have caused a crash before the null pointer test if it'd ever occur, so presumably it doesn't.
|
I want to peek at each of these in a touch more detail I merge on this one. thanks for all of these! if you would like I would love to see what you find when you run across the shortcircuit-xt codebase we have here too! |
|
@baconpaul I've had a brief look at the shortcircuit-xt code and have added comments/issues to a couple. Many of the reported issues are common with this codebase, so it's currently difficult to see the wood for the trees. |
|
Great. Is there a sort of 'raw dumped output' from the tool as a massive text file we could have on scxt? or does it not work that way. These are super useful! |
|
@baconpaul It can save its log in a variety of formats (txt, csv, html, json) but I only ever use it interactively from the VS IDE so I'm not sure how easy it is to work with them. It's best to have static analysis always on the go to keep on top of any issues, otherwise it's an enormous task to manage. |
|
yeah i wonder if the tool integrates with clion yet. I'd be happy to look at an html doc or whatever though to get an idea of the sort of things it is showing! I merged a few other of your changes already this morning by the wya. |
|
Yes, it appears that you can use it from CLion https://pvs-studio.com/en/docs/manual/0052/ |
|
SCXT is just a shorthand for the other repo, Shortcircuit XT. |
src/common/PatchDB.cpp
Outdated
| @@ -839,7 +839,6 @@ CREATE TABLE IF NOT EXISTS Favorites ( | |||
| std::ostringstream searchName; | |||
| searchName << p.name << " "; | |||
|
|
|||
| if (storage) | |||
There was a problem hiding this comment.
The static analyzer is sort of correct here, that storage is used above, but it is only used in an exception path. The correct fix I think is not to remove this guard, but to guard the report errors above. I'll make that change on the branch.
There was a problem hiding this comment.
@baconpaul IIRC there are many places where it's used unguarded and I couldn't think why it should only not be null only in exception paths - which was why I assumed it probably never is null in practice!
Looking again, the constructor of WriterWorker would crash if it was passed a nullptr.
There was a problem hiding this comment.
In this function it's only used prior in the exception path. so i flipped it to check the exception paths
Those exceptions are pretty rare so I think if storage was null those cases would crash.
I sort of wish there was an easy non-nullable pointer type. I guess that's a & but they are clumsy to mutate.
There was a problem hiding this comment.
The constructor on L413 would crash were it ever passed a nullptr for storage.
Reworking using a ref for storage seems to build fine for me.
There was a problem hiding this comment.
Yeah i think you are correct.
Since synth : storage : ui are all 1:1 in surge right now, we really should have set everything up as a & or const&. That's more what we are doing in short circuit. Many of the 2004 bones are still on display in the raw pointer idiom here.
|
@baconpaul |
|
Oh yeah that would be perfect. Just attach a CSV dump to an issue as a zip file. I keep meaning to get around to applying for an open source license for the tool but just been a really nutty winter. |
Remove null pointer checks when the pointer had already been used. The code would have caused a crash before the null pointer test if it'd ever occur, so presumably it doesn't.