Eric Blake <ebl...@redhat.com> writes:
> On 01/30/2018 04:21 AM, Markus Armbruster wrote:
>> This cleanup makes the number of objects depending on qapi/error.h
>> drop from 1910 (out of 4739) to 1612 in my "build everything" tree.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> arch_init.c | 1 +
>> audio/wavcapture.c | 1 +
>> balloon.c | 1 +
>> block.c | 2 ++
>> block/block-backend.c | 1 +
>> block/iscsi.c | 1 +
>> block/qapi.c | 1 +
>
> So several .c files have to use it explicitly,
>
>> fsdev/qemu-fsdev-throttle.h | 1 -
>
>> include/crypto/random.h | 1 -
>> include/crypto/xts.h | 1 -
>> include/hw/ide/internal.h | 1 -
>> include/ui/console.h | 1 -
>
> because they were previously getting it from .h files that don't
> directly emit an error. Makes sense.
>
> Patches like this are easy to test - if it still compiles, you did it
> right ;) Out of curiousity, how are you counting how many files got
> compiled per run? Touch the .h, then pass 'make' output to 'grep -c "^
> CC "'?
The data is up for grabs: in the .d gcc spits out for make. I process
them like this, after fresh build:
$ find bld -name \*.d -exec cat {} + | clean-deps
where clean-deps is this AWK script:
#!/usr/bin/awk -f
/\\$/ {
l = $0
while (sub(/\\$/, "", l) && (getline t))
l = l t
$0 = l
}
NF > 1 {
delete a
for (i = 2; i <= NF; i++) {
n = $i
# ignore system headers and non-headers
if (match(n, /^\/usr/) || !match(n, /\.h$/))
continue
# strip leading ../ from target build running in sub-directory
sub(/^\.\.\//, "", n)
# normalize /../ and /./
while (sub(/[^\/]*\/\.\.\//, "", n)) ;
gsub(/\/\.\//, "/", n)
# absolute -> relative
sub(/\/home\/armbru\/work\//, "/work/armbru/", n)
sub(/\/work\/armbru\/qemu\/(bld\/)?/, "", n)
# ignore dupes
if (n in a)
continue
a[n] = i
h[n]++
}
}
END {
for (i in h)
print i, h[i]
}
My conversion from absolute to relative is specific to my personal
setup. It would have to be generalized before we can put this into
scripts.
> But just to make sure nothing weird is happening, I also read through
> it, and found:
>
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index a0b8ed6167..0fb8093f92 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -21,6 +21,7 @@
>> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> * THE SOFTWARE.
>> */
>> +
>> #include "qemu/osdep.h"
>> #include "qemu-common.h"
>> #include "cpu.h"
>
> Spurious whitespace change. Should this belong in 1/18, even though
> arch_init.c didn't need cleanup there? Or...
>
>> +++ b/block.c
>> @@ -21,6 +21,7 @@
>> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> * THE SOFTWARE.
>> */
>> +
>> #include "qemu/osdep.h"
>> #include "block/trace.h"
>> #include "block/block_int.h"
>
> ...since you did it again, do you need a separate patch for ALL of these
> types of whitespace cleanups near osdep.h?
I routinely insert this blank line when I touch includes anyway.
>> +++ b/block/qcow2.c
>> @@ -21,6 +21,7 @@
>> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> * THE SOFTWARE.
>> */
>> +
>> #include "qemu/osdep.h"
>> #include "block/block_int.h"
>> #include "sysemu/block-backend.h"
>
> and again
>
>> +++ b/chardev/char-ringbuf.c
>> @@ -21,9 +21,11 @@
>> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> * THE SOFTWARE.
>> */
>> +
>> #include "qemu/osdep.h"
>> #include "chardev/char.h"
>> #include "qmp-commands.h"
>> +#include "qapi/error.h"
>> #include "qemu/base64.h"
>>
>> /* Ring buffer chardev */
>
> Here, it's in the same hunk, so a bit more forgivable. But there's
> definitely enough of them that a separate commit might be in order.
>
> At any rate, whether done as one patch (with a better commit message) or
> as two, I see nothing semantically wrong with the cleanups done here, so
I'll amend the commit message.
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks!
