> On Oct 7, 2021, at 1:43 PM, Marvin Häuser <[email protected]> wrote:
>
> Hey Mike,
> Hey Andrew,
>
> I'll just reply to both mails at once :)
>
> On 07/10/2021 19:36, Andrew Fish wrote:
>>
>>
>>> On Oct 7, 2021, at 1:19 PM, Michael D Kinney <[email protected]>
>>> wrote:
>>>
>>> Hi Marvin,
>>>
>>> Some comments below.
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From:[email protected]<[email protected]> On Behalf Of Marvin Häuser
>>>> Sent: Thursday, October 7, 2021 11:31 AM
>>>> To: Leif Lindholm
>>>> <[email protected]>;[email protected];[email protected]
>>>> Cc:[email protected]; Michael Kubacki <[email protected]>;
>>>> Bret Barkelew <[email protected]>;
>>>> Kinney, Michael D <[email protected]>
>>>> Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?
>>>>
>>>> Good day,
>>>>
>>>> +1, but while you're at it, can we have arguments not align to the
>>>> function name...
>>>>
>>>> Status = Test (
>>>> a
>>>> );
>>>>
>>>> ... but to the next natural indentation level?
>>>>
>>>> Status = Test (
>>>> a
>>>> );
>>>>
>>>> Basically no IDE I have seen supports EDK II's style, and I wouldn't be
>>>> keen on writing known-broken style to then rely on Uncrustify to fix it.
>>>>
>>>> I also have heard some controversy regarding casts off-list, where some
>>>> prefer no spaces after casts to stress the evaluation order, and some
>>>> prefer spaces to have clearer visuals (as a cast *ideally* would be
>>>> something rare that requires good justification). Just throwing that out
>>>> there.
>>>>
>>>>
>>>> For things unrelated to autoformat (so semi-offtopic) but still relevant
>>>> to the coding spec:
>>>>
>>>> 1. Allow STATIC functions (if the debugging concerns are still relevant,
>>>> there could be another level of indirection, like RELEASE_STATIC)?
>>>
>>> Debugging concerns are no longer relevant. The suggestion in the BZ below
>>> is to remove the STATIC macro and allow EDK II sources to add 'static'
>>> to any functions it makes sense to use on.
>>>
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1766
>
> Thanks! I'd keep STATIC actually just for the sake of not doing no-op changes
> that do not really do anything and for consistency with CONST, but whatever
> works really.
>
>>>
>>>>
>>>> 2. Allow variable assignments on definition (basically non-static CONST
>>>> variables are banned...)?
>>>
>>> Are referring to use of pre-initialized CONST variables declared within
>>> a function? I think Bret brought this topic up when implementing some
>>> unit tests and the suggestion to pass ECCC was to promote them to
>>> pre-initialized CONST global variables.
>
> Yes.
>
>>>
>>> The challenges we have seen in the past with pre-initialized variables
>>> within
>>> a function is that they can cause compilers to inject use of memcpy() calls,
>>> especially if the variable being initialized on the stack is a structure.
>>> These cause build breaks today.
>>
>> This issue is independent of CONST. I’m not sure a coding style tool is
>> smart enough to catch this generically? You need an understanding of C types
>> to know if the local variable assignment is going to trigger a memcpy().
>>
>> What I’ve seen in the real world is the firmware compiles with -Os or LTO to
>> fit int he ROM for DEBUG and RELEASE, and the optimizer optimizes away the
>> call to memcpy. Then if you try to build NOOPT (or over ride the compiler
>> flags on an individual driver/lib) you fail to link as only the NOOPT build
>> injects the memcpy.
>
> +1
>
>>
>> Thus I think the best way to enforce this rule is to compile a project
>> NOOPT. I’m trying to remember are there flags to built to tell it to compile
>> and skip the FD construction? Maybe we should advocate platforms add a NOOPT
>> build target that just compiles the code, but does not create the FD?
>
> I know there were stability concerns with intrinsics in the past, but
> memcpy() is in the standard, and the rest remained stable to my knowledge.
> Maybe it's time to fix the issues at the root? Works for us:
> https://github.com/acidanthera/OpenCorePkg/tree/master/Library/OcCompilerIntrinsicsLib
>
> <https://github.com/acidanthera/OpenCorePkg/tree/master/Library/OcCompilerIntrinsicsLib>
>
Marvin,
Good point. This would make the rule moot. So maybe just removing the
requirement would be the easiest long term fix.
Other embedded projects I know of do this too, and as you point out the
compilers keep these APIs standard for folks the provide their own runtimes.
Thanks,
Andrew Fish
> Best regards,
> Marvin
>
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>>
>>>>
>>>> 3. Allow variable declarations at any scope (I had some nasty shadowing
>>>> bugs before, probably prohibit shadowing with warnings)?
>>>
>>> By shadowing do you mean the declaration of the same variable name in
>>> multiple scoped within the same function?
>>>
>>>>
>>>> 4. Require that exactly all function declarations and all function
>>>> definitions with no prior declaration must be documented (first
>>>> direction is enforcing docs, second is prohibiting doc duplication, I've
>>>> seen them go out-of-sync plenty of times)?
>>>
>>> I agree that this can reduce duplication and sync issues. The uncrustify
>>> tool being discussed here could not help clean this up or enforce this
>>> type of rule. It is a good topic, but may need to be split out into its
>>> own thread.
>>>
>>>>
>>>> The latter bunch would not require any autoformat rules or reformatation
>>>> of existing code, but would be target only new submissions in my
>>>> opinion. Thoughts?
>>>>
>>>>
>>>> Thanks for your efforts!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> Am 07.10.2021 um 12:48 schrieb Leif Lindholm:
>>>>> Hi Michael,
>>>>>
>>>>> Apologies, I've owed you a response (promised off-list) for a while
>>>>> now.
>>>>>
>>>>> First, let me say I hugely appreciate this effort. Apart from aligning
>>>>> the codebase(s), this will reduce manual reviewing effort
>>>>> substantially, as well as cutting down on number of rework cycles for
>>>>> developers.
>>>>>
>>>>> Looking at the changes to (well, the comments in) uncrustify, this
>>>>> seems to be constrained to:
>>>>> - Newline after '(' for multi-line function calls.
>>>>> - Dealing with "(("/"))" for DEBUG macros.
>>>>> - Function pointer typedefs:
>>>>> - typedef\nEFIAPI
>>>>> - closing parentheses indentation
>>>>>
>>>>> I don't think I've made any secret over the years that I am not a
>>>>> massive fan of the EDK2 coding style in general. So I think for any
>>>>> of its quirks that are substantial enough that they require not just
>>>>> custom configuration but actual new function added to existing code
>>>>> conformance tools, this would be an excellent point to sanitise the
>>>>> coding style instead.
>>>>>
>>>>> Taking these in order:
>>>>>
>>>>> Newline after '('
>>>>> -----------------
>>>>> I think we already reached a level of flexibility around this, where
>>>>> we don't actually enforce this (or single argument per
>>>>> line). Personally, I'd be happy to update the coding style as
>>>>> required instead.
>>>>>
>>>>> DEBUG macro parentheses
>>>>> -----------------------
>>>>> How does uncrustify treat DEBUG macros without this modification?
>>>>> Do we start getting everything turned into multi-level indented
>>>>> multi-line statements without this change?
>>>>>
>>>>> Function pointer typedefs:
>>>>> --------------------------
>>>>> I don't see that function pointer typedefs need to rigidly follow the
>>>>> same pattern as the declaration of functions implementing them. Could
>>>>> we update the coding style (if needed) instead?
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> Leif
>>>>>
>>>>> On Mon, Aug 16, 2021 at 16:00:38 -0400, Michael Kubacki wrote:
>>>>>> The edk2 branch was created here:
>>>>>> https://github.com/makubacki/edk2/tree/uncrustify_poc_2
>>>>>>
>>>>>> We have a Project Mu fork with custom changes to the Uncrustify tool to
>>>>>> help
>>>>>> comply with EDK II formatting here:
>>>>>> https://dev.azure.com/projectmu/_git/Uncrustify
>>>>>>
>>>>>> The latest information about the status and how to experiment with the
>>>>>> configuration file and the tool are in that fork:
>>>> https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wiki/1/Project-Mu-(EDK-II)-Fork-Readme
>>>>>>
>>>>>> That said, I have also finished a CI plugin to run Uncrustify that
>>>>>> should be
>>>>>> ready soon to initially deploy in Project Mu. Before doing so, I am
>>>>>> trying
>>>>>> to settle on an initial configuration file that less strictly but more
>>>>>> reliably formats the code than in the examples in those branches. For
>>>>>> example, remove heuristics that when run against the same set of code
>>>>>> multiple times can produce different results. An example would be a rule
>>>>>> that reformats code because it exceeds a specified column width on one
>>>>>> run
>>>>>> but on the next run that reformatted code triggers a different rule to
>>>>>> further align the code and so on. At least initially, some rules might be
>>>>>> tweaked in a more conservative approach that can be tightened in the
>>>>>> future.
>>>>>> Once this configuration file is ready, we will baseline Project Mu code
>>>>>> as
>>>>>> an example and turn on the plugin. The CI plugin runs Uncrustify against
>>>>>> modified files and if there's any changes, indicating a formatting
>>>>>> deviation, the diff chunks are saved in a log so they can be viewed as a
>>>>>> build artifact.
>>>>>>
>>>>>> I am making progress on the updated config file and I should be able to
>>>>>> post
>>>>>> a "uncrustify_poc_3" branch soon with the results.
>>>>>>
>>>>>> Regarding indentation, Marvin is right that Uncrustify cannot support
>>>>>> edk2
>>>>>> indentation style out-of-box. Some changes are made in that fork to
>>>>>> handle
>>>>>> the formatting. At this point, it can handle the indentation in the cases
>>>>>> I've seen. Uncrustify does potentially give us the ability to massively
>>>>>> deploy changes across the codebase in case a decision were made to change
>>>>>> the style.
>>>>>>
>>>>>> Thanks,
>>>>>> Michael
>>>>>>
>>>>>> On 8/16/2021 3:39 PM, Marvin Häuser wrote:
>>>>>>> Hey Rebecca,
>>>>>>>
>>>>>>> I think even Uncrustify has issues with the EDK II indentation style.
>>>>>>> You might want to check the UEFI Talkbox Discord server, I had a brief
>>>>>>> chat with Michael about it there. I don't think realistically any tool
>>>>>>> supports EDK II's indentation style however, so I'd propose it is
>>>>>>> changed. This could be for new submissions only, or actually the entire
>>>>>>> codebase could be reformatted at once with a good tool setup. While this
>>>>>>> screws with git blame, the (to my understanding) decided on CRLF -> LF
>>>>>>> change does that anyway, so at least two evils could be dealt with in
>>>>>>> one go really.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Marvin
>>>>>>>
>>>>>>> On 16/08/2021 21:34, Rebecca Cran wrote:
>>>>>>>>
>>>>>>>> cc devel@ .
>>>>>>>>
>>>>>>>> On 8/16/21 1:33 PM, Rebecca Cran wrote:
>>>>>>>>>
>>>>>>>>> I noticed a message on Twitter about an idea of using Uncrustify
>>>>>>>>> for EDK2 instead of the ECC tool, and came across https://www.mail-
>>>> archive.com/[email protected]&q=subject:%22Re%5C%3A+%5C%5Bedk2%5C-
>>>> devel%5C%5D+TianoCore+Community+Meeting+Minutes+%5C-+2%5C%2F4%22&o=newest&f=1
>>>>>>>>> .
>>>>>>>>>
>>>>>>>>> I was wondering if there's been any progress on it that I could
>>>>>>>>> check out?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Michael Kubacki: in that message, you said:
>>>>>>>>>
>>>>>>>>> "I'm planning to put up a branch that we can use as a reference
>>>>>>>>> for a conversation around uncrustify in the next couple of
>>>>>>>>> weeks."
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Did you end up creating that branch, and if so could you provide
>>>>>>>>> a link to it please?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Rebecca Cran
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81613): https://edk2.groups.io/g/devel/message/81613
Mute This Topic: https://groups.io/mt/84932137/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
