NOISSUE - Refactoring geiger lib and adding further testing #158
Conversation
jmcconnell26
force-pushed
the
NOISSUE-RefactorGeigerLib
branch
from
01aa1d3
to
805b5cf
Compare
geiger/src/geiger_syn_visitor.rs
Outdated
| pub struct GeigerSynVisitor { | ||
| /// Count unsafe usage inside tests | ||
| pub include_tests: IncludeTests, | ||
|
|
||
| /// The resulting data from a single file scan. | ||
| pub metrics: RsFileMetrics, | ||
|
|
||
| /// The number of nested unsafe scopes that the GeigerSynVisitor are | ||
| /// currently in. For example, if the visitor is inside an unsafe function | ||
| /// and inside an unnecessary unsafe block inside that function, then this | ||
| /// number should be 2. If the visitor is outside unsafe scopes, in a safe | ||
| /// scope, this number should be 0. | ||
| /// This is needed since unsafe scopes can be nested and we need to know | ||
| /// when we leave the outmost unsafe scope and get back into a safe scope. | ||
| pub unsafe_scopes: u32, | ||
| } |
There was a problem hiding this comment.
Can we keep the members on this struct private or will that result in too much bloat?
There was a problem hiding this comment.
Ahh, it looks like the include_tests and unsafe_scopes can definitely be (my bad being too keen with the refactoring). I think the metrics needs to be public at the crate level (it's called in the newly created find module), but it should be restricted to that by https://github.com/rust-secure-code/cargo-geiger/pull/158/files#diff-a49da58edbe4631a734dcd7884f75469e5ee8ab7859659643656a113c9b2ee81R11 however I'm more than happy to add a pub crate restriction to it, to make explicit the exposure level?
There was a problem hiding this comment.
I'm more than happy to add a pub crate restriction to it, to make explicit the exposure level?
Not needed on my opinion. The reason I wanted to restrict the other members was to avoid accidental coupling between different modules down the line. Keeping internal API:s inside a library clear and minimal tends to make maintenance and refactoring easier in the long run, in my experience.
| .count() | ||
| > 0 |
There was a problem hiding this comment.
Since CI passes this is following rustfmt standard, but still very strange formatting style to use a newline here imho... :)
There was a problem hiding this comment.
I actually went back and had a go at setting it explicitly to .count() > 0 but it appears cargo fmt seems to update it to be like this.
Do you think its worth looking into the cargo fmt settings, or do you reckon it's OK like this?
There was a problem hiding this comment.
If this is what rustfmt does, let's go with that.
* Making inner members of struct geiger_syn_visitor private where possible Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
jmcconnell26
force-pushed
the
NOISSUE-RefactorGeigerLib
branch
from
805b5cf
to
897baa9
Compare
Signed-off-by: joshmc josh-mcc@tiscali.co.uk