r212591 - clang-format: Revamp function declaration/definition indentation.

[Daniel Jasper]

On Wed, Jul 9, 2014 at 11:08 AM, Tobias Grosser <tobias at grosser.es> wrote:

> On 09/07/2014 10:47, Daniel Jasper wrote:
>
>> Yes, this is intended and I actually don't think that it is such a big
>> difference. Multiline variable declaration that are broken after the type
>> are actually reasonably rare.
>>
>
> In Polly alone it changes 180 lines (43 declarations)
>
> LLVM has 276 command line declarations that are currently broken
> after the type (grep -R 'cl::opt<.*>$' lib/ | wc):
>
> static cl::opt<bool>
> VarName("options, ...")
>
> and 232 that are formatted


a) That is not a lot (<1% of the codebase).
b) I think volume of change is actually not all that relevant when deciding
on what formatting should be.


> static cl::opt<std::string> OnlyFunction(
>      "polly-only-func")
>
> If you look at the last five commit messages that explicitly mention
> clang-format three contain formatting changes of exactly these constructs:
>
> r212143:
>
> -static cl::list<std::string> ArchFlags("arch",
> -    cl::desc("architecture(s) from a Mach-O file to dump"),
> -    cl::ZeroOrMore);
> +static cl::list<std::string>
> +ArchFlags("arch", cl::desc("architecture(s) from a Mach-O file to dump"),
> +          cl::ZeroOrMore);
>
>
> r212141:
>
> -static cl::opt<OutputFormatTy>
> -       OutputFormatShort(cl::desc("Specify output format"),
> -         cl::values(clEnumValN(sysv, "A", "System V format"),
> -                    clEnumValN(berkeley, "B", "Berkeley format"),
> -                    clEnumValN(darwin, "m", "Darwin -m format"),
> -                    clEnumValEnd),
> -         cl::init(berkeley));
> +OutputFormat("format", cl::desc("Specify output format"),
> +             cl::values(clEnumVal(sysv, "System V format"),
> +                        clEnumVal(berkeley, "Berkeley format"),
> +                        clEnumVal(darwin, "Darwin -m format"),
> clEnumValEnd),
> +             cl::init(berkeley));
>
> r210182
>
> -static cl::opt<bool>
> -EnableAArch64UnscaledMemOp("aarch64-unscaled-mem-op", cl::Hidden,
> -                         cl::desc("Allow AArch64 unscaled load/store
> combining"),
> -                         cl::init(true));
> +static cl::opt<bool> EnableAArch64UnscaledMemOp(
> +    "aarch64-unscaled-mem-op", cl::Hidden,
> +    cl::desc("Allow AArch64 unscaled load/store combining"),
> cl::init(true));
>
>
>  Happy, to take a vote on this though..
>>
>
> Sure, would it be OK to revert this change meanwhile to get the build bot
> clean again.
>

No. I think it is highly likely, we are going to stick with this change.
Moreover, I think the issue here is that we have a buildbot enforcing a
format based on clang-format's HEAD version. Lets do one of:
- Remove formatting test from buildbot. I think formatting is important,
but it is good enough to clean this up every now and then, e.g. in a
manually invoked step. Having incorrect indentation for a few days doesn't
do any harm. That would also give room to separate formatting-only commits
from other commits.
- Use a released version (or other known good version) of clang-format for
these tests. We need to decouple clang-format commits from the cleanup of
polly.

Cheers,
Daniel


>
> Cheers,
> Tobias
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140709/92635b81/attachment.html>