Skip to content

Conversation

@fingolfin
Copy link
Member

This PR is somewhat dual to PR #3183 resp. #3202: it adds static to as many functions and variables as possible. This helps to find dead code (see e.g. PR #3203), may help compilers to do better optimizations, and also is a step towards defining a larger "libgap" API, by "hiding" away stuff that we do not explicitly want to export (this makes it harder to "cheat" and call those functions anyway; now people who want to do that hopefully will instead talk to us to get us to export them, giving us a chance to define a proper API).

The second commit is optional and clang-formats the first commit, based on the idea that if we touch those lines anyway, we might as well reformat them, too.

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 20, 2019
@fingolfin fingolfin force-pushed the mh/static branch 4 times, most recently from 21b65c1 to 6082556 Compare January 22, 2019 16:11
@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@e868e29). Click here to learn what that means.
The diff coverage is 86.24%.

@@            Coverage Diff            @@
##             master    #3204   +/-   ##
=========================================
  Coverage          ?   84.76%           
=========================================
  Files             ?      687           
  Lines             ?   335795           
  Branches          ?        0           
=========================================
  Hits              ?   284634           
  Misses            ?    51161           
  Partials          ?        0
Impacted Files Coverage Δ
src/vector.c 92.73% <ø> (ø)
src/vars.c 93.3% <ø> (ø)
src/vecgf2.c 73.3% <ø> (ø)
src/weakptr.c 98.24% <ø> (ø)
src/vecffe.c 71.02% <ø> (ø)
src/vec8bit.c 83.91% <ø> (ø)
src/objcftl.c 23.86% <0%> (ø)
src/integer.c 98.43% <100%> (ø)
src/bool.c 98.94% <100%> (ø)
src/code.c 96.28% <100%> (ø)
... and 52 more

@coveralls
Copy link

coveralls commented Jan 22, 2019

Coverage Status

Coverage decreased (-0.0003%) to 84.643% when pulling e8dee1e on fingolfin:mh/static into e868e29 on gap-system:master.

@ChrisJefferson
Copy link
Contributor

Did you check this didn't break compiling / loading any packages?

@fingolfin
Copy link
Member Author

@ChrisJefferson the testpackages test on Travis verifies that all distributed packages still compile and load. This testing really works: it even found one issue in an earlier revision of this PR: the cvec package unfortunately has extern unsigned long PolsFF[]; so I had to drop static from PolsFF -- at the same time, this is a nice example for exactly the kind of thing I'd like to prevent in the future: I'll work on fixing cvec to not require this (possibly by adding a suitable new API to the GAP kernel). Actually, I was already aware of this before, see gap-packages/cvec#17. I'll try to get it addressed in time for GAP 4.11

@fingolfin fingolfin merged commit 4402d59 into gap-system:master Jan 23, 2019
@fingolfin fingolfin deleted the mh/static branch January 23, 2019 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants