Conversation
| val fuzzEngine: String = FUZZ_ENGINE_DEFAULT, | ||
| val hooks: Boolean = HOOKS_DEFAULT, | ||
| val keepGoing: Int = KEEP_GOING_DEFAULT, | ||
| val instrument: List<String>, |
There was a problem hiding this comment.
Why do we want to specialise instrument why not use an empty list by default?
There was a problem hiding this comment.
Because, without instrumentation jazzer (and other coverage-guided fuzzers) doesn't work properly. So, I believe, it's better to explicitly ask user to set it to empty list.
Idealy, we could deduct the instrument automatically. In that case, we should let the user not to specify it. I believe, we'll do it later.
FerrumBrain
left a comment
FerrumBrain
left a comment
There was a problem hiding this comment.
Looks good to me, however I would wait for approval from Denis
ilma4
added
in progress
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ilma4
added
ready for review
AbdullinAM
left a comment
AbdullinAM
left a comment
There was a problem hiding this comment.
Overall everything looks good. But I think maybe we should first resolve and merge #26. Then we will be able to merge this and ensure that everything is consistent
# Conflicts: # kotlinx.fuzz.api/build.gradle.kts
DLochmelis33
left a comment
DLochmelis33
left a comment
There was a problem hiding this comment.
I have one major question: why did you decide to ditch a simple interface with methods like storeAsSystemProperties() / loadFromSystemProperties()? (even though I agree that custom delegates are cool 😎 )
| val toString: (T) -> String, | ||
| val fromString: (String) -> T, | ||
| ) : ReadWriteProperty<Any, T> { | ||
| private var cachedValue: T? = null |
There was a problem hiding this comment.
I guess not because of writes, but see my next comment before resolving this
| error("Property '${property.name}' is already set") | ||
| } | ||
| cachedValue = value | ||
| } |
There was a problem hiding this comment.
To be honest, the logic seems unnecessarily complicated to me. When we read a property, we will have lazy init from system properties, but when we write a property, we won't write them into system, so we need to do an explicit dump of our config, but not an explicit load. But wait, we have fromSystemProperties(), so we actually do need to do an explicit load? And I don't see a method for explicit write to system properties, which I just said we should do?... I'm confused 🤔
| fuzzEngine = "twice" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
do we also need to test whether the config is properly stored and loaded from system properties? (because right now I'm not even sure how to do it...)
I didn't do Regarding general complexity: the main idea was to make sure DSL is always correct with the minimal effort. In the end, there is only 2 ways to get a |
…w, `KFuzzConfigImpl.fromSystemProperties` explicitly sets each property
I guess that from the user's POV this is the only thing that matters, and we will only need to use it in the code one time. My only concern is that this code has a bus factor of 1 👀 but it's not particularly complicated, so it should be fine. |
|
@DLochmelis33 |
4 participants
No description provided.