This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Diff 2 - Separate FuzzerDef.h in appropriate header files.
ClosedPublic

Authored by mpividori on Nov 29 2016, 3:17 PM.

Details

Summary

Split the declarations in FuzzerDefs.h into the files: FuzzerIO.h, FuzzerUtil.h and FuzzerSHA1.h.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 79653.Nov 29 2016, 3:17 PM
mpividori retitled this revision from to [libFuzzer] Diff 2 - Separate FuzzerDef.h in appropriate header files..
mpividori updated this object.
mpividori added reviewers: kcc, zturner.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.
kcc edited edge metadata.Nov 29 2016, 4:27 PM

I don't mind, but please at least explain the rationale.
Also, I see some leftovers from other patches, e.g. GetSeparator().
Please try to separate "refactoring, no-functionality change" patches from "change functionality" patches.

mpividori updated this revision to Diff 79686.Nov 29 2016, 6:30 PM
mpividori updated this object.
mpividori edited edge metadata.

@kcc . Yes, you are right, I had included some changes here that should go in the next diff (Diff 3). I updated this diff to remove that changes. (And I will include that changes in the Diff 3).
In this diff, I only split the declarations in FuzzerDefs.h into the files: FuzzerIO.h, FuzzerUtil.h and FuzzerSHA1.h.

zturner added inline comments.Nov 30 2016, 8:45 AM
lib/Fuzzer/FuzzerCorpus.h
20 ↗(On Diff #79686)

Did you run clang-format on this patch? It's supposed to alphabetize headers, but these are not in alphabetical order here.

mpividori added inline comments.Nov 30 2016, 8:59 AM
lib/Fuzzer/FuzzerCorpus.h
20 ↗(On Diff #79686)

@zturner, thanks for your comment. I haven't run clang-format. I think libFuzzer need many changes to be according to LLVM Coding Standards. If you agree, I will add all of that changes in a new diff.

kcc accepted this revision.Nov 30 2016, 9:04 AM
kcc edited edge metadata.

LGTM

I think I know why you need this, but please update the description explicitly explaining the rationale.
And sort the headers as zturner@ asks.

This revision is now accepted and ready to land.Nov 30 2016, 9:04 AM
zturner added inline comments.Nov 30 2016, 9:14 AM
lib/Fuzzer/FuzzerCorpus.h
20 ↗(On Diff #79686)

I got offline confirmation from kcc@ that running clang-format is not necessary. So you don't need to run clang-format

mpividori updated this revision to Diff 79763.Nov 30 2016, 9:18 AM
mpividori edited edge metadata.

Update diff. Improve order of "#includes".

@kcc , I separated the header file because it is better for code organization. Separating the functionalities in different logic units makes the code easier to understand and modify. So, each implementation only includes the interface of the unit that it needs to access, not all the declarations in one header file.
If we have only one header file like FuzzerDefs.h, when we need to look at the implementation of particular function, we don't have any idea in which file was implemented. Also, all implementations files include all the declarations in FuzzerDefs, so it is not clear which is the actual relation between the different implementations.

This revision was automatically updated to reflect the committed changes.