Fixes from static analysis #4391
Conversation
vlstill
force-pushed
the
static-analysis-fixes
branch
from
daffbdb to
43e31df
Compare
vlstill
force-pushed
the
static-analysis-fixes
branch
from
43e31df to
52610df
Compare
| @@ -530,7 +530,7 @@ const IR::Node *AssertsParser::postorder(IR::P4Table *node) { | |||
| auto *cloneProperties = node->properties->clone(); | |||
| IR::IndexedVector<IR::Property> properties; | |||
| for (const auto *property : cloneProperties->properties) { | |||
| if (property->name.name != "key" || property->name.name != "entries") { | |||
| if (property->name.name != "key" && property->name.name != "entries") { | |||
There was a problem hiding this comment.
@fruffy, note that the original condition was a tautology. I've changed it to match the comment above.
There was a problem hiding this comment.
Thanks for catching this. Hope I can get rid of this pass soon.
| @@ -104,7 +104,7 @@ const IR::Vector<IR::Expression> *RemoveComplexExpressions::simplifyExpressions( | |||
| // of the list. Otherwise we simplify the argument itself. | |||
| // This is mostly for functions that take FieldLists - these | |||
| // should still take a list as argument. | |||
| bool changes = true; | |||
| bool changes = false; | |||
There was a problem hiding this comment.
Note the for cycle below.
|
It might make more sense to go over the PR commit-by-commit as each commit is a separate problem fix. |
lib/options.cpp
Outdated
| @@ -15,7 +15,8 @@ limitations under the License. | |||
| */ | |||
|
|
|||
| #include "options.h" | |||
| #include <lib/null.h> | |||
|
|
|||
| #include "null.h" | |||
There was a problem hiding this comment.
"lib/null.h" should work. When something like #include <lib/null.h> happens it usually means some tool (clang-tidy or IWYU) adds system includes instead of local includes.
There was a problem hiding this comment.
I was mainly surprised this failed in bazel, but not everywhere else. This seems like some sort of inconsistency in the build system, but since I don't know bazel I didn't want to investigate it.
There was a problem hiding this comment.
Ah, the quotes in my comment got swallowed.
The lack of support for angled brackets is by design: https://bazel.build/docs/bazel-and-cpp#include-paths
| @@ -123,6 +123,8 @@ class ReadsWrites : public Inspector { | |||
| void postorder(const IR::Operation_Binary *expression) override { | |||
| auto left = ::get(rw, expression->left); | |||
| auto right = ::get(rw, expression->right); | |||
| CHECK_NULL(left); | |||
There was a problem hiding this comment.
We use this a lot. Maybe we should implement some form of checked ::get?
There was a problem hiding this comment.
Maybe... or just use .at() it does produce a standard exception instead of p4c one, but that is not a big problem in my opinion. Maybe we should have some sort of guidelines. I'd guess many new programmers don't even know there is ::get until they find some in the code.
vlstill
force-pushed
the
static-analysis-fixes
branch
from
6f63277 to
fb97ca1
Compare
|
@fruffy, I've fixed the include. The |
…its source location
vlstill
force-pushed
the
static-analysis-fixes
branch
from
fb97ca1 to
8950d8d
Compare
Yeah, that would be great if there is a good free analyzer. It seems that clang-tidy is quite a bit more limited then coverity, and it is pretty slow as you have pointed out. Maybe there would be a way to run in in scheduled runs (nightly, weekly), or in some sort of differential mode though. I don't know about other options sadly. |
I will take a look. P4Testgen will fail because of a failing test that slipped past CI. #4397 should fix it, feel free to merge that one, patch the fix, or just ignore the failure. |
|
It looks like the auto merger is willing to auto-merge with failed optional tests... Is this deliberate? |
Well, I also did not expect that. But it makes sense, there might be optional tests that fail frequently and you do not want to make the auto-merge useless because of that. The tools tests are still optional. |
Fixes for issues found by coverity.