Re: Using clang-format

[Tom Tromey <tom@tromey.com>]

>> gcc_type
>> compile_cplus_instance::convert_reference_base (
>> gcc_type base, enum gcc_cp_ref_qualifiers rquals)
>> 
>> But really, I don't think these should be blockers.  It would take
>> someone some really bad faith to say that the above changes how they
>> read the code.

I don't agree, mainly because I dislike that style, and I feel like I'm
generally above-board.

This weird dangling paren was an issue for calls, previously, though,
and I fixed it by tweaking one of the penalty settings.  So maybe that
can be salvaged.

>> I have only seen this when we make clang-format's life difficult by
>> using long names or long string literals.  If we don't like how it
>> formats some piece of code, we always have the choice of tweaking it to
>> make its like easier.

Yeah.  Also it's not like the current code is pristine.

>> I'm surprised you didn't use "UseTab: Always"

I routinely forget whether gdb uses tabs or not and rely on Emacs to
DTRT.

>> I would use "IndentPPDirectives: AfterHash".  Unlike what we use
>> currently, it will indent with $IndentWidth columns, whereas we
>> typically we indent these with a single column when we do it manually.
>> But, I think I prefer 2 columns actually.  You can try formatting
>> gdbsupport/gdb_locale.h for a good example.

My impression was that normally gdb didn't do this kind of indentation,
but in this particular case, I don't really have any opinion and I'm
happy with whatever setting everyone else enjoys.

>> Speaking of which, in order to be usable from gdbsupport and gdbserver,
>> .clang-format would need to be in the top-level directory.  Or maybe it
>> could be in gdbsupport, and gdb/gdbserver symlink to it.

Yeah, I was wondering about this as well.

I don't think we should put it at top-level, though, as people may think
it applies to the whole tree, which it wouldn't.

>> btrace_block (CORE_ADDR begin, CORE_ADDR end) : begin (begin), end (end)
>> {
>> /* Nothing.  */
>> }
>> 
>> The initializer list is put on the same line as the constructor's
>> prototype.  If there's a way to tell it that we don't want that, it
>> would be closer to our style.

I think clang-format doesn't offer this level of control here.

>> For AlignEscapedNewlines, I'd probably choose Right or DontAlign.  The
>> reason being that if you modify the longest line of a macro (make that
>> line longer or shorter), all the macros line will change as a result.
>> So it will be harder to spot what changed when reading the hunk in the
>> diff.  I have a slight preference for Right,

Sounds good.

>>> 1. It removes all the Control-L page breaks from the source.
>>> I know these often confuse newcomers, so maybe it's fine.
>>> I feel that Someone out there will cheer this :)
>> 
>> Who dat?

I thought Pedro was in the anti-control-L camp?
Anyway, no biggie, I like them but I probably won't notice their
absence.

>>> Another possible problem is that, unlike with 'black', ensuring that
>>> everyone has the same version is a pain.

>> With the Python code, we did a massive re-format.  I don't think it did
>> / will cause a big inconvenience, because there aren't many patches to
>> the Python code.  But for the C++ code, it might be different, as there
>> are more patches in everyone's pipeline that touch that code.

If our reformatting commit is purely automated, we can write a short
script using git filter-branch so that anybody with a patch to rebase
can first rebase to the commit just before the reformat, then run the
script to rebase-and-reformat.  This would ease the pain somewhat.

>> Another question is: would we re-format the C/C++ code in the testsuite?
>> It might require a few adjustments to test cases (like there was one for
>> black), but I would be fine with that.  My concern is more if there are
>> some tests where the formatting of the code was specifically important.
>> We could disable formatting for them, but the difficulty is to find
>> them.

Personally I would say that we shouldn't touch the tests at all, and
just not worry about the formatting there.  That's been my usual review
policy, as I think it's fine, and maybe even mildly beneficial in some
theoretical way, for the tests to have a variety of styles.  Also
normally they are just throwaway code anyway.

Tom