This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Encapsulate commands in a class
ClosedPublic

Authored by aarongreen on Nov 15 2017, 2:55 PM.

Details

Summary

To be more portable (especially w.r.t. platforms without system()), commands should be managed programmatically rather than via string manipulation on the command line. This change introduces Fuzzer::Command, with methods to manage arguments and flags, set output options, and execute the command.

Diff Detail

Repository
rL LLVM

Event Timeline

aarongreen created this revision.Nov 15 2017, 2:55 PM
kcc edited edge metadata.

Nice, thank you!
Do I understand correctly that this is a no-functional-change-intended change?

Some comments below.
Matt, please make the next round of reviews (once my comments are addressed) and land it when done. (Assuming Aaron doesn't have commit access).

lib/fuzzer/FuzzerCommand.cpp
53 ↗(On Diff #123086)

here and below:
LLVM coding style prefers to not use {} for single-statement if-body

lib/fuzzer/FuzzerCommand.h
21 ↗(On Diff #123086)

consider haing all of this class in the header. There is not too much code in it and splitting it into t .h and .cpp bloats the code.

21 ↗(On Diff #123086)

s/haing/having

22 ↗(On Diff #123086)

Does this class deserve a gunit-style unit test in lib/fuzzer/tests/FuzzerUnittest.cpp ?

morehouse added inline comments.Nov 16 2017, 12:40 PM
lib/fuzzer/FuzzerCommand.cpp
38 ↗(On Diff #123086)

LLVM style prefers first letter of parameters and locals to be capitalized. (here and below)

The one common exception I've seen is for one-letter loop indexes to be lowercase (though some people capitalize those too).

aarongreen updated this revision to Diff 125235.Dec 1 2017, 3:29 PM
aarongreen marked 4 inline comments as done.

Holidays, illnesses, and more have kept me from closing on this; apologies.

Yes, kcc, this change is supposed maintain functional equivalence. It should only provide a more convenient way to build processes from commands without having to do a bunch of string parsing.

I've tried to go through and match the style guide. I'm still familiarizing myself with that one, please correct me if I'm not compliant. I also added the unit tests. Finally, I changed ExecuteCommand to take a Command rather than a string since the change was small enough that I don't think it warrants a separate review.

Thanks for the review!

morehouse accepted this revision.Dec 4 2017, 10:05 AM

LGTM. Do you have commit access, or should I land this for you?

This revision is now accepted and ready to land.Dec 4 2017, 10:05 AM

To the best of my knowledge, I do not have commit access, and believe I would know if I did . :)

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.