Re: [edk2] (spawning off more style discussion)

2018-04-18 Thread Laszlo Ersek
On 04/17/18 15:55, Leif Lindholm wrote:
> On Mon, Apr 16, 2018 at 09:32:44PM +0200, Laszlo Ersek wrote:
>> (4) Please consider adopting
>>
>>   
>> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10
>>   
>> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
>>
>> so that patches roughly advance from "abstract" to "concrete".
> 
> I have been reluctant to adopt this aspect of the guidance since it
> means project-specific syntax to generic commands (whereas
> --stat-graph-width and --stat are generic and not always needed).
> 
> However, at least newer versions of git seem to support a
> diff.orderFile config option. So you should see this improved in
> future patches from me.

Indeed, when I wrote the wiki article, there was only -O, and no
diff.orderFile. This annoyed me to no end, and when diff.orderFile was
brought about by a new git release (I don't know which one), I was happy
to set it. Obviously, I never updated the wiki article. That's the bad
thing about documentation, it unavoidably rots, and creates a
never-ending obligation for updates.

> 
>> (6) Please don't use "__" and "_C" prefixes for identifiers (see
>> __CONCATENATE and _CONCATENATE); according to the C standard, "All
>> identifiers that begin with an underscore and either an uppercase
>> letter or another underscore are always reserved for any use".
>>
>> I know Linux uses "__" prefixes liberally; that doesn't make them
>> any less wrong :)
> 
> I stole those internal names from MdePkg/Include/Base.h

Ouch! :)

> While that makes me a double-baddie - should we have some global
> macros for this particularly awkward bit of CPP-ness?

Yes. I think turning them into the public CONCAT_WORKER() and CONCAT()
macros should be fine. There's no reason not to use ## wherever it makes
sense, so Base.h should support it. (The same argument worked for
ARRAY_SIZE() as well.)

>> ... Obviously I don't insist on these patches being implemented "my
>> way"; I'm stating my opinion because you CC'd me :) (Thanks for that!)
> 
> And your input is always appreciated.

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] (spawning off more style discussion)

2018-04-17 Thread Leif Lindholm
On Mon, Apr 16, 2018 at 09:32:44PM +0200, Laszlo Ersek wrote:
> (4) Please consider adopting
> 
>   
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10
>   
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> 
> so that patches roughly advance from "abstract" to "concrete".

I have been reluctant to adopt this aspect of the guidance since it
means project-specific syntax to generic commands (whereas
--stat-graph-width and --stat are generic and not always needed).

However, at least newer versions of git seem to support a
diff.orderFile config option. So you should see this improved in
future patches from me.

> (6) Please don't use "__" and "_C" prefixes for identifiers (see
> __CONCATENATE and _CONCATENATE); according to the C standard, "All
> identifiers that begin with an underscore and either an uppercase
> letter or another underscore are always reserved for any use".
> 
> I know Linux uses "__" prefixes liberally; that doesn't make them
> any less wrong :)

I stole those internal names from MdePkg/Include/Base.h
While that makes me a double-baddie - should we have some global
macros for this particularly awkward bit of CPP-ness?

> ... Obviously I don't insist on these patches being implemented "my
> way"; I'm stating my opinion because you CC'd me :) (Thanks for that!)

And your input is always appreciated.

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel