Re: [PATCH 2/4] Add an include-checking script

[Tom Tromey <tromey@adacore.com>]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +def failure(filename, ndx, text):
>> +    print(filename + ":" + str(ndx + 1) + ": " + text)

Simon> Should this print to stderr?

Done.

>> +    global status
>> +    status = 1

Simon> I would prefer if failure returned 1 (or the callers could return 1
Simon> themselves).

I ended up having it just call sys.exit (which really just throws).
The approach in the patch was from an earlier version that was run by
hand, before I figured out it could be run from pre-commit; and there
working on multiple files was more important.

>> +def headers(dirname):
>> +    return glob.iglob(dirname + "/*.h")

Simon> This function appears to be unused.

Removed.

Simon> You could do:
Simon>   contents = [line.strip('\n') for line in f]
Simon> and then no longer have to deal with \n everywhere.

>> +    if force_rewrite:
>> +        contents[i] = "#ifndef " + expected + "\n"

Simon> You don't need to add a \n here (and at other spots where you write a
Simon> line), writelines will do it.

The Python docs seem to say otherwise:

https://docs.python.org/3/library/io.html#io.IOBase.writelines

I left all the \n stuff alone.

>> +    if contents[i] != "#endif /* " + expected + " */\n":
>> +        failure(filename, i, "incorrect endif")
>> +        contents[i] = "#endif /* " + expected + " */\n"
>> +        updated = True

Simon> You could access the last line with contents[-1].

I figured we needed the line number for a possible error message anyway.

Tom