Skip to content

Ensure everything compiles individually & sort includes #9

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

Merged
merged 2 commits into from
Jun 27, 2017

Conversation

aleksejspopovs
Copy link
Collaborator

A lot of the files in our codebase don't properly include all of their dependencies. A common example of this is a situation like this: a.hpp uses std::vector in a definition, so it would need to have a line saying #include <vector>, but it doesn't. a.hpp does not include vector neither directly nor indirectly, but whenever a.hpp is included from another file, that file just happens to include vector beforehand.

The result is that, while running make succeeds, the code is quite brittle. The fact that vector is always included before a.hpp (in current uses of the library) might be quite accidental, and users might in theory want to use a.hpp in another file that doesn't need vector itself. Besides, it's just not particularly good style to not include our dependencies explicitly.

Luckily, this specific problem is easy to detect: if you were to try to compile a.hpp on its own, compilation would not succeed. I wrote a script (try_compiling.py in libsnark-tooling) that tries to individually compile every single file (*.hpp or *.cpp) in libff and reports whether it succeeds.

I then went through the report and fixed every single compilation error --- almost all of them were because of missing includes.

This does not resolve all possible include-related problems: in the situation above, perhaps a.hpp actually has a line saying #include <libff/b.hpp>, and b.hpp includes vector --- then a.hpp compiles just fine, but is still not explicitly listing all of its dependencies (perhaps b.hpp's dependencies will change in the future, and it will no longer require vector --- then we'll get a broken a.hpp despite no real changes inside it). But I think this is a much smaller problem, and it's also much harder to even detect automatically, so for now I'm submitting just these changes.

While I was at it, I also sorted the #include directives in every file --- they are now grouped into three groups (standard library, external libraries, and libff) and sorted lexicographically within each group. This was done using fix-includes.py in libsnark-tooling.

Another minor change I made is that libff/common/default_types/ec_pp.hpp will now refuse to compile (with a helpful error message) if none of the CURVE_* symbols are defined, because that file is always included with the expectation that it will define default_ec_pp.

I have added a couple missing includes, so that every *.cpp or *.hpp
file in the repository can now be compiled individually. I also grouped
all includes (by stdlib/external libs/libff) and sorted them.
@tromer
Copy link
Member

tromer commented Jun 27, 2017

Looks great, and indeed needed. Let's merge.

@madars madars merged commit 1352290 into scipr-lab:master Jun 27, 2017
mobileink added a commit to minatools/libff that referenced this pull request Jul 20, 2020
# This is the 1st commit message:

bazel support, initial commit

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#2:

gitignore .bazelrc, bazel-*

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#3:

remove sha256 from rule_foreign_cc rule

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#4:

rename target ff to libff

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#5:

put curves/mnt/mnt4, 6 in separate packages

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#6:

bn128: drop "depends" from include prefix, for bazel compatibility

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#7:

fix refs to targets mnt4, mnt6, libff

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#8:

fix refs to @ate_pairing//:libgmp

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#9:

change @// to //

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#10:

delete obsolete mnt4, mnt6 targets from curves/mnt/BUILD.bazel

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#11:

add target scalar_multiplication:multiexp_profile

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#12:

list headers explicitly

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#13:

dead code elim

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#14:

dead code elim

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#15:

BUILD files: explicitate srcs/hdrs, DCE, buildifier reformat

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#16:

change @ate_pairing//:zm to @ate_pairing//ate-pairing

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#17:

switch obazl repos from local to git

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#18:

pin xbyak, ate-pairing repos to versions, to match upstream

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#19:

delete git submodules, not needed with Bazel

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#20:

add sha256 for xbyak, ate-pairing external repos

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#21:

remove xbyak dep - it's included in ate-pairing

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#22:

restore depends dirs

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>
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.

3 participants