Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code cleanup #906

Merged
merged 8 commits into from
Jan 24, 2023
Merged

Code cleanup #906

merged 8 commits into from
Jan 24, 2023

Conversation

arcz
Copy link
Member

@arcz arcz commented Jan 13, 2023

This PR features various cleanup and simplifications in the entirety of code:

  • Use OverloadedRecordDot as much as possible
  • Use </> operator for path joining
  • Use Sets instead of (NE)lists. This is good for generating random data as picking random elements from a large Set is way faster than lists
  • Rewrite some complex one-liners to something that we can understand 😄
  • Remove underscores from most of the structures and Template Haskell generated lenses. I think we can even go entirely without lenses as, in practice, we don't really do that much nested data manipulation and I'm not sure a few more lines of code are worth the added cost of knowing lenses. However, if we still find lenses useful, we should move on to a more modern solution such as generic-lens or optics-core in the future.

@@ -12,21 +12,18 @@ import System.Directory (createDirectoryIfMissing, makeRelativeToCurrentDirector
import Echidna.Types.Tx
import Echidna.Output.Utils

saveTxs :: Maybe FilePath -> [[Tx]] -> IO ()
saveTxs (Just d) txs = mapM_ saveTx txs where
saveTxs :: FilePath -> [[Tx]] -> IO ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change correct? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe is now matched earlier:

  -- save corpus
  case cfg._cConf._corpusDir of
    Nothing -> pure ()
    Just dir -> do
      saveTxs (dir </> "reproducers") (filter (not . null) $ (.testReproducer) <$> cpg._tests)
      saveTxs (dir </> "coverage") (snd <$> Set.toList cpg._corpus)

@@ -104,16 +106,18 @@ removeJsonFiles dir =
let path = dir </> file
whenM (doesFileExist path) $ removeFile path

addresses :: SolConf -> NE.NonEmpty AbiValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NonEmpty has different properties than Set, so I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there were a non-empty set, but Set is better for what we do here as we pick random elements and list/non-empty list has slow random access. We would need to convert lists to sets, that takes linear time which kills the performance during fuzzing.

@ggrieco-tob
Copy link
Member

I'm getting this error when trying to save a corpus:

echidna-test: corpus-new/covered./1674496096/.txt: withFile: does not exist (No such file or directory)

@arcz
Copy link
Member Author

arcz commented Jan 23, 2023

Fixed. Apparently, I got confused by the variable names so fixed them too.

@ggrieco-tob ggrieco-tob merged commit dceee29 into master Jan 24, 2023
@ggrieco-tob ggrieco-tob deleted the cleanup branch January 24, 2023 14:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants