|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: Clang-format configuration discussion - pt 2
> On 24 Nov 2023, at 12:47, Jan Beulich <jbeulich@xxxxxxxx> wrote: > > On 23.11.2023 15:47, Luca Fancellu wrote: >> Hi all, >> >> Let’s continue the discussion about clang-format configuration, this is part >> 2, previous discussions are: >> >> - https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg00498.html >> >> You can find the serie introducing clang-format here: >> https://patchwork.kernel.org/project/xen-devel/cover/20231031132304.2573924-1-luca.fancellu@xxxxxxx/ >> and there is also a patch linked to my gitlab account where you can find the >> output for the hypervisor code. >> >> For a full list of configurables and to find the possible values for them, >> please refer to this page: >> https://clang.llvm.org/docs/ClangFormatStyleOptions.html >> >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> Our coding style doesn’t mention anything about alignment, shall we add a >> new section? >> I can send patches when we reach agreement on each of these rules. >> >> >> QualifierAlignment: Custom >> QualifierOrder: ['static', 'inline', 'const', 'volatile', 'type'] >> >> --- >> For “QualifierAlignment” I chose Custom in order to apply in >> “QualifierOrder” an order for the >> qualifiers that match the current codebase, we could specify also “Leave” in >> order to keep >> them as they are. > > Where do attributes go in this sequence? I think function declaration/definition and variables. > >> Depending on how the discussion goes on this one, it could be an entry in >> our coding style >> >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> AlignAfterOpenBracket: Align >> >> --- >> This one is to align function parameters that overflows the line length, I >> chose to align them >> to the open bracket to match the current codebase (hopefully) >> >> e.g.: >> someLongFunction(argument1, >> argument2); > > The above matches neither of the two generally permitted styles: > > someLongFunction(argument1, > argument2); > > someLongFunction( > argument1, > argument2); > > Then again from its name I would infer this isn't just about function > arguments? I think it applies to parameters and arguments of functions and macro, given the description in the docs. I see your two snippets above but I’ve always found at least on arm a predominance of the style above for functions, so arguments aligned after the opening bracket, for macros there is a mix. I might be wrong though and so another opinion from another maintainer would help. In any case we can choose among many value: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignafteropenbracket, but when we do so, we need to stick to one permitted style only, the tool don’t allow to specify more than one. > >> This one can be a candidate for an entry in our coding style >> >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> AlignArrayOfStructures: Left >> >> --- >> “When using initialization for an array of structs aligns the fields into >> columns." >> It’s important to say that even if we specify “None”, it is going to format >> the data structure anyway, >> I choose left, but clearly I’m open to suggestions. > > You don't say in which way it re-formats such constructs. Sure, taking as example an array of structure, xen/drivers/video/modelines.h, With AlignArrayOfStructures: None we have this below. diff --git a/xen/drivers/video/modelines.h b/xen/drivers/video/modelines.h index 9cb7cdde055f..3ff23ef1f8a7 100644 --- a/xen/drivers/video/modelines.h +++ b/xen/drivers/video/modelines.h @@ -42,36 +42,36 @@ struct modeline { }; struct modeline __initdata videomodes[] = { - { "640x480@60", 25175, 640, 16, 96, 48, 480, 11, 2, 31 }, - { "640x480@72", 31500, 640, 24, 40, 128, 480, 9, 3, 28 }, - { "640x480@75", 31500, 640, 16, 96, 48, 480, 11, 2, 32 }, - { "640x480@85", 36000, 640, 32, 48, 112, 480, 1, 3, 25 }, - { "800x600@56", 38100, 800, 32, 128, 128, 600, 1, 4, 14 }, - { "800x600@60", 40000, 800, 40, 128, 88 , 600, 1, 4, 23 }, - { "800x600@72", 50000, 800, 56, 120, 64 , 600, 37, 6, 23 }, - { "800x600@75", 49500, 800, 16, 80, 160, 600, 1, 2, 21 }, - { "800x600@85", 56250, 800, 32, 64, 152, 600, 1, 3, 27 }, - { "1024x768@60", 65000, 1024, 24, 136, 160, 768, 3, 6, 29 }, - { "1024x768@70", 75000, 1024, 24, 136, 144, 768, 3, 6, 29 }, - { "1024x768@75", 78750, 1024, 16, 96, 176, 768, 1, 3, 28 }, - { "1024x768@85", 94500, 1024, 48, 96, 208, 768, 1, 3, 36 }, - { "1280x1024@60", 108000, 1280, 48, 112, 248, 1024, 1, 3, 38 }, - { "1280x1024@75", 135000, 1280, 16, 144, 248, 1024, 1, 3, 38 }, - { "1280x1024@85", 157500, 1280, 64, 160, 224, 1024, 1, 3, 44 }, - { "1400x1050@60", 122610, 1400, 88, 152, 240, 1050, 1, 3, 33 }, - { "1400x1050@75", 155850, 1400, 96, 152, 248, 1050, 1, 3, 42 }, - { "1600x1200@60", 162000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@65", 175500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@70", 189000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@75", 202500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@85", 229500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1792x1344@60", 204800, 1792, 128, 200, 328, 1344, 1, 3, 46 }, - { "1792x1344@75", 261000, 1792, 96, 216, 352, 1344, 1, 3, 69 }, - { "1856x1392@60", 218300, 1856, 96, 224, 352, 1392, 1, 3, 43 }, - { "1856x1392@75", 288000, 1856, 128, 224, 352, 1392, 1, 3, 104 }, - { "1920x1200@75", 193160, 1920, 128, 208, 336, 1200, 1, 3, 38 }, - { "1920x1440@60", 234000, 1920, 128, 208, 344, 1440, 1, 3, 56 }, - { "1920x1440@75", 297000, 1920, 144, 224, 352, 1440, 1, 3, 56 }, + { "640x480@60", 25175, 640, 16, 96, 48, 480, 11, 2, 31 }, + { "640x480@72", 31500, 640, 24, 40, 128, 480, 9, 3, 28 }, + { "640x480@75", 31500, 640, 16, 96, 48, 480, 11, 2, 32 }, + { "640x480@85", 36000, 640, 32, 48, 112, 480, 1, 3, 25 }, + { "800x600@56", 38100, 800, 32, 128, 128, 600, 1, 4, 14 }, + { "800x600@60", 40000, 800, 40, 128, 88, 600, 1, 4, 23 }, + { "800x600@72", 50000, 800, 56, 120, 64, 600, 37, 6, 23 }, + { "800x600@75", 49500, 800, 16, 80, 160, 600, 1, 2, 21 }, + { "800x600@85", 56250, 800, 32, 64, 152, 600, 1, 3, 27 }, + { "1024x768@60", 65000, 1024, 24, 136, 160, 768, 3, 6, 29 }, + { "1024x768@70", 75000, 1024, 24, 136, 144, 768, 3, 6, 29 }, + { "1024x768@75", 78750, 1024, 16, 96, 176, 768, 1, 3, 28 }, + { "1024x768@85", 94500, 1024, 48, 96, 208, 768, 1, 3, 36 }, + { "1280x1024@60", 108000, 1280, 48, 112, 248, 1024, 1, 3, 38 }, + { "1280x1024@75", 135000, 1280, 16, 144, 248, 1024, 1, 3, 38 }, + { "1280x1024@85", 157500, 1280, 64, 160, 224, 1024, 1, 3, 44 }, + { "1400x1050@60", 122610, 1400, 88, 152, 240, 1050, 1, 3, 33 }, + { "1400x1050@75", 155850, 1400, 96, 152, 248, 1050, 1, 3, 42 }, + { "1600x1200@60", 162000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@65", 175500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@70", 189000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@75", 202500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@85", 229500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1792x1344@60", 204800, 1792, 128, 200, 328, 1344, 1, 3, 46 }, + { "1792x1344@75", 261000, 1792, 96, 216, 352, 1344, 1, 3, 69 }, + { "1856x1392@60", 218300, 1856, 96, 224, 352, 1392, 1, 3, 43 }, + { "1856x1392@75", 288000, 1856, 128, 224, 352, 1392, 1, 3, 104 }, + { "1920x1200@75", 193160, 1920, 128, 208, 336, 1200, 1, 3, 38 }, + { "1920x1440@60", 234000, 1920, 128, 208, 344, 1440, 1, 3, 56 }, + { "1920x1440@75", 297000, 1920, 144, 224, 352, 1440, 1, 3, 56 }, }; #endif With AlignArrayOfStructures: Left we have (I noticed there might be a small bug in clang-format since the first entry has no space after the opening curly bracket): diff --git a/xen/drivers/video/modelines.h b/xen/drivers/video/modelines.h index 9cb7cdde055f..1afe725dcb4c 100644 --- a/xen/drivers/video/modelines.h +++ b/xen/drivers/video/modelines.h @@ -42,36 +42,36 @@ struct modeline { }; struct modeline __initdata videomodes[] = { - { "640x480@60", 25175, 640, 16, 96, 48, 480, 11, 2, 31 }, - { "640x480@72", 31500, 640, 24, 40, 128, 480, 9, 3, 28 }, - { "640x480@75", 31500, 640, 16, 96, 48, 480, 11, 2, 32 }, - { "640x480@85", 36000, 640, 32, 48, 112, 480, 1, 3, 25 }, - { "800x600@56", 38100, 800, 32, 128, 128, 600, 1, 4, 14 }, - { "800x600@60", 40000, 800, 40, 128, 88 , 600, 1, 4, 23 }, - { "800x600@72", 50000, 800, 56, 120, 64 , 600, 37, 6, 23 }, - { "800x600@75", 49500, 800, 16, 80, 160, 600, 1, 2, 21 }, - { "800x600@85", 56250, 800, 32, 64, 152, 600, 1, 3, 27 }, - { "1024x768@60", 65000, 1024, 24, 136, 160, 768, 3, 6, 29 }, - { "1024x768@70", 75000, 1024, 24, 136, 144, 768, 3, 6, 29 }, - { "1024x768@75", 78750, 1024, 16, 96, 176, 768, 1, 3, 28 }, - { "1024x768@85", 94500, 1024, 48, 96, 208, 768, 1, 3, 36 }, - { "1280x1024@60", 108000, 1280, 48, 112, 248, 1024, 1, 3, 38 }, - { "1280x1024@75", 135000, 1280, 16, 144, 248, 1024, 1, 3, 38 }, - { "1280x1024@85", 157500, 1280, 64, 160, 224, 1024, 1, 3, 44 }, - { "1400x1050@60", 122610, 1400, 88, 152, 240, 1050, 1, 3, 33 }, - { "1400x1050@75", 155850, 1400, 96, 152, 248, 1050, 1, 3, 42 }, - { "1600x1200@60", 162000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@65", 175500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@70", 189000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@75", 202500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@85", 229500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1792x1344@60", 204800, 1792, 128, 200, 328, 1344, 1, 3, 46 }, - { "1792x1344@75", 261000, 1792, 96, 216, 352, 1344, 1, 3, 69 }, - { "1856x1392@60", 218300, 1856, 96, 224, 352, 1392, 1, 3, 43 }, - { "1856x1392@75", 288000, 1856, 128, 224, 352, 1392, 1, 3, 104 }, - { "1920x1200@75", 193160, 1920, 128, 208, 336, 1200, 1, 3, 38 }, - { "1920x1440@60", 234000, 1920, 128, 208, 344, 1440, 1, 3, 56 }, - { "1920x1440@75", 297000, 1920, 144, 224, 352, 1440, 1, 3, 56 }, + {"640x480@60", 25175, 640, 16, 96, 48, 480, 11, 2, 31 }, + { "640x480@72", 31500, 640, 24, 40, 128, 480, 9, 3, 28 }, + { "640x480@75", 31500, 640, 16, 96, 48, 480, 11, 2, 32 }, + { "640x480@85", 36000, 640, 32, 48, 112, 480, 1, 3, 25 }, + { "800x600@56", 38100, 800, 32, 128, 128, 600, 1, 4, 14 }, + { "800x600@60", 40000, 800, 40, 128, 88, 600, 1, 4, 23 }, + { "800x600@72", 50000, 800, 56, 120, 64, 600, 37, 6, 23 }, + { "800x600@75", 49500, 800, 16, 80, 160, 600, 1, 2, 21 }, + { "800x600@85", 56250, 800, 32, 64, 152, 600, 1, 3, 27 }, + { "1024x768@60", 65000, 1024, 24, 136, 160, 768, 3, 6, 29 }, + { "1024x768@70", 75000, 1024, 24, 136, 144, 768, 3, 6, 29 }, + { "1024x768@75", 78750, 1024, 16, 96, 176, 768, 1, 3, 28 }, + { "1024x768@85", 94500, 1024, 48, 96, 208, 768, 1, 3, 36 }, + { "1280x1024@60", 108000, 1280, 48, 112, 248, 1024, 1, 3, 38 }, + { "1280x1024@75", 135000, 1280, 16, 144, 248, 1024, 1, 3, 38 }, + { "1280x1024@85", 157500, 1280, 64, 160, 224, 1024, 1, 3, 44 }, + { "1400x1050@60", 122610, 1400, 88, 152, 240, 1050, 1, 3, 33 }, + { "1400x1050@75", 155850, 1400, 96, 152, 248, 1050, 1, 3, 42 }, + { "1600x1200@60", 162000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@65", 175500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@70", 189000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@75", 202500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1600x1200@85", 229500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, + { "1792x1344@60", 204800, 1792, 128, 200, 328, 1344, 1, 3, 46 }, + { "1792x1344@75", 261000, 1792, 96, 216, 352, 1344, 1, 3, 69 }, + { "1856x1392@60", 218300, 1856, 96, 224, 352, 1392, 1, 3, 43 }, + { "1856x1392@75", 288000, 1856, 128, 224, 352, 1392, 1, 3, 104}, + { "1920x1200@75", 193160, 1920, 128, 208, 336, 1200, 1, 3, 38 }, + { "1920x1440@60", 234000, 1920, 128, 208, 344, 1440, 1, 3, 56 }, + { "1920x1440@75", 297000, 1920, 144, 224, 352, 1440, 1, 3, 56 }, }; #endif With AlignArrayOfStructures: Right we have: diff --git a/xen/drivers/video/modelines.h b/xen/drivers/video/modelines.h index 9cb7cdde055f..539ab7c12d00 100644 --- a/xen/drivers/video/modelines.h +++ b/xen/drivers/video/modelines.h @@ -42,36 +42,36 @@ struct modeline { }; struct modeline __initdata videomodes[] = { - { "640x480@60", 25175, 640, 16, 96, 48, 480, 11, 2, 31 }, - { "640x480@72", 31500, 640, 24, 40, 128, 480, 9, 3, 28 }, - { "640x480@75", 31500, 640, 16, 96, 48, 480, 11, 2, 32 }, - { "640x480@85", 36000, 640, 32, 48, 112, 480, 1, 3, 25 }, - { "800x600@56", 38100, 800, 32, 128, 128, 600, 1, 4, 14 }, - { "800x600@60", 40000, 800, 40, 128, 88 , 600, 1, 4, 23 }, - { "800x600@72", 50000, 800, 56, 120, 64 , 600, 37, 6, 23 }, - { "800x600@75", 49500, 800, 16, 80, 160, 600, 1, 2, 21 }, - { "800x600@85", 56250, 800, 32, 64, 152, 600, 1, 3, 27 }, - { "1024x768@60", 65000, 1024, 24, 136, 160, 768, 3, 6, 29 }, - { "1024x768@70", 75000, 1024, 24, 136, 144, 768, 3, 6, 29 }, - { "1024x768@75", 78750, 1024, 16, 96, 176, 768, 1, 3, 28 }, - { "1024x768@85", 94500, 1024, 48, 96, 208, 768, 1, 3, 36 }, - { "1280x1024@60", 108000, 1280, 48, 112, 248, 1024, 1, 3, 38 }, - { "1280x1024@75", 135000, 1280, 16, 144, 248, 1024, 1, 3, 38 }, - { "1280x1024@85", 157500, 1280, 64, 160, 224, 1024, 1, 3, 44 }, - { "1400x1050@60", 122610, 1400, 88, 152, 240, 1050, 1, 3, 33 }, - { "1400x1050@75", 155850, 1400, 96, 152, 248, 1050, 1, 3, 42 }, - { "1600x1200@60", 162000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@65", 175500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@70", 189000, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@75", 202500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1600x1200@85", 229500, 1600, 64, 192, 304, 1200, 1, 3, 46 }, - { "1792x1344@60", 204800, 1792, 128, 200, 328, 1344, 1, 3, 46 }, - { "1792x1344@75", 261000, 1792, 96, 216, 352, 1344, 1, 3, 69 }, - { "1856x1392@60", 218300, 1856, 96, 224, 352, 1392, 1, 3, 43 }, - { "1856x1392@75", 288000, 1856, 128, 224, 352, 1392, 1, 3, 104 }, - { "1920x1200@75", 193160, 1920, 128, 208, 336, 1200, 1, 3, 38 }, - { "1920x1440@60", 234000, 1920, 128, 208, 344, 1440, 1, 3, 56 }, - { "1920x1440@75", 297000, 1920, 144, 224, 352, 1440, 1, 3, 56 }, + { "640x480@60", 25175, 640, 16, 96, 48, 480, 11, 2, 31}, + { "640x480@72", 31500, 640, 24, 40, 128, 480, 9, 3, 28}, + { "640x480@75", 31500, 640, 16, 96, 48, 480, 11, 2, 32}, + { "640x480@85", 36000, 640, 32, 48, 112, 480, 1, 3, 25}, + { "800x600@56", 38100, 800, 32, 128, 128, 600, 1, 4, 14}, + { "800x600@60", 40000, 800, 40, 128, 88, 600, 1, 4, 23}, + { "800x600@72", 50000, 800, 56, 120, 64, 600, 37, 6, 23}, + { "800x600@75", 49500, 800, 16, 80, 160, 600, 1, 2, 21}, + { "800x600@85", 56250, 800, 32, 64, 152, 600, 1, 3, 27}, + { "1024x768@60", 65000, 1024, 24, 136, 160, 768, 3, 6, 29}, + { "1024x768@70", 75000, 1024, 24, 136, 144, 768, 3, 6, 29}, + { "1024x768@75", 78750, 1024, 16, 96, 176, 768, 1, 3, 28}, + { "1024x768@85", 94500, 1024, 48, 96, 208, 768, 1, 3, 36}, + {"1280x1024@60", 108000, 1280, 48, 112, 248, 1024, 1, 3, 38}, + {"1280x1024@75", 135000, 1280, 16, 144, 248, 1024, 1, 3, 38}, + {"1280x1024@85", 157500, 1280, 64, 160, 224, 1024, 1, 3, 44}, + {"1400x1050@60", 122610, 1400, 88, 152, 240, 1050, 1, 3, 33}, + {"1400x1050@75", 155850, 1400, 96, 152, 248, 1050, 1, 3, 42}, + {"1600x1200@60", 162000, 1600, 64, 192, 304, 1200, 1, 3, 46}, + {"1600x1200@65", 175500, 1600, 64, 192, 304, 1200, 1, 3, 46}, + {"1600x1200@70", 189000, 1600, 64, 192, 304, 1200, 1, 3, 46}, + {"1600x1200@75", 202500, 1600, 64, 192, 304, 1200, 1, 3, 46}, + {"1600x1200@85", 229500, 1600, 64, 192, 304, 1200, 1, 3, 46}, + {"1792x1344@60", 204800, 1792, 128, 200, 328, 1344, 1, 3, 46}, + {"1792x1344@75", 261000, 1792, 96, 216, 352, 1344, 1, 3, 69}, + {"1856x1392@60", 218300, 1856, 96, 224, 352, 1392, 1, 3, 43}, + {"1856x1392@75", 288000, 1856, 128, 224, 352, 1392, 1, 3, 104}, + {"1920x1200@75", 193160, 1920, 128, 208, 336, 1200, 1, 3, 38}, + {"1920x1440@60", 234000, 1920, 128, 208, 344, 1440, 1, 3, 56}, + {"1920x1440@75", 297000, 1920, 144, 224, 352, 1440, 1, 3, 56}, }; #endif > >> I don’t know how to phrase this one in our coding style >> >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> AlignConsecutiveAssignments: None >> >> --- >> This one is disabled because of feedbacks from Stefano and Alejandro about >> some weird behaviour on our >> codebase. >> >> This one could be phased along this line: “Consecutive assignments don't >> need to be aligned.”, the issue is >> that in this way it seems that it’s optional, but clang-format is going to >> remove the alignment anyway for >> assignment that are consecutive and aligned. > > Like below if there's no way to say "leave alone", then I don't consider this > usable. Imo we want to permit people to align when they think it helps, but > we don't want to demand them doing so. Yes I understand, unfortunately the tool doesn’t allow that and if we don’t specify this configurable, it will take the default one which is disabled (and so would remove the formatting from already aligned assignments that it would eventually find). This most of the configurable, when not specified, they apply their default to the code anyway. > > Jan > >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> AlignConsecutiveBitFields: None >> >> --- >> Same thing as AlignConsecutiveAssignments, but for bitfields. >> >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> AlignConsecutiveDeclarations: None >> >> --- >> This aligns declarations names, same considerations as >> AlignConsecutiveAssignments. >> >> -------------------------------------------------------------------------------------------------------------------------------------------------- >> >> Ok this is it for now, let me know your thoughts about them, ideally if I >> don’t get any reply in two weeks (7th of December), >> I will consider that we have an agreement on these configuration. >> >> Cheers, >> Luca >
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |