Thread: Make #else/#endif comments more consistent

Hello,

while reading the postgres code, occasionally I see a little bit of 
inconsistency in the comments after #else (and corresponding #endif).

In some places #else/endif's comment expresses condition for else block 
to be active:
#ifdef HAVE_UUID_OSSP
...
#else                           /* !HAVE_UUID_OSSP */
...
#endif                          /* HAVE_UUID_OSSP */

and in others -- just the opposite:

#ifdef SHA2_UNROLL_TRANSFORM
...
#else                           /* SHA2_UNROLL_TRANSFORM */
...
#endif                          /* SHA2_UNROLL_TRANSFORM */

Also, #endif comment after #else might expresses condition for else 
block to be active:
#ifdef USE_ICU
...
#else                           /* !USE_ICU */
...
#endif                          /* !USE_ICU */

or it might be just the opposite, like in HAVE_UUID_OSSP and 
SHA2_UNROLL_TRANSFORM examples above.


I propose making them more consistent. Would the following guidelines be 
acceptable?


1. #else/#elif/#endif's comment, if present, should reflect the
condition of the #else/#elif block as opposed to always being a copy
of #if/ifdef/ifndef condition.

e.g. prefer this:
#if LLVM_VERSION_MAJOR > 11
...
#else                           /* LLVM_VERSION_MAJOR <= 11 */
...
#endif                          /* LLVM_VERSION_MAJOR <= 11 */

over this:

#if LLVM_VERSION_MAJOR > 11
...
#else                           /* LLVM_VERSION_MAJOR > 11 */
...
#endif                          /* LLVM_VERSION_MAJOR > 11 */


2. In #else/#elif/#endif comments, prefer A to defined(A).

E.g. prefer this:
#endif                          /* DMETAPHONE_MAIN */
over
#endif                          /* defined DMETAPHONE_MAIN */

And this:
#else                           /* !_MSC_VER */
over
#else                           /* !defined(_MSC_VER) */


3. Textual hand-crafted condition comments are perfectly fine.
Like this:
#else                           /* no ppoll(), so use select() */


4. #else/#endif condition comment, if present, should reflect the
*effective* condition, i.e. condition taking into account previous
#if/#elif-s.

E.g. do this:
#if defined(HAVE_INT128)
...
#elif defined(HAS_64_BIT_INTRINSICS)
...
#else                           /* !HAVE_INT128 && !HAS_64_BIT_INTRINSICS */
...
#endif                          /* !HAVE_INT128 && !HAS_64_BIT_INTRINSICS */


5. Comment of the form "!A && !B", if deemed complicated enough, may
also be expressed as "neither A nor B" for easier reading.

Example:
#if (defined(HAVE_LANGINFO_H) && defined(CODESET)) || defined(WIN32)
...
#else                           /* neither (HAVE_LANGINFO_H && CODESET) 
nor WIN32 */
...
#endif                          /* neither (HAVE_LANGINFO_H && CODESET) 
nor WIN32 */


6. Use "!" as opposed to "not" to be consistent. E.g. do this:
#ifdef LOCK_DEBUG
...
#else                           /* !LOCK_DEBUG */
...
#endif                          /* !LOCK_DEBUG */

as opposed to:

#ifdef LOCK_DEBUG
...
#else                           /* not LOCK_DEBUG */
...
#endif                          /* not LOCK_DEBUG */


The draft of proposed changes is attached as
0001-Make-else-endif-comments-more-consistent.patch
In the patch I've also cleaned up some minor things, like removing
occasional "//" comments within "/* */" ones.

Any thoughts?
-- 
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

On 29.08.22 11:38, Anton Voloshin wrote:
> I propose making them more consistent. Would the following guidelines be 
> acceptable?

I usually try to follow the guidelines in 
<https://www.gnu.org/prep/standards/html_node/Comments.html>, which 
pretty much match what you are proposing.

> And this:
> #else                           /* !_MSC_VER */
> over
> #else                           /* !defined(_MSC_VER) */

Note that for _MSC_VER in particular there is some trickiness: We 
generally use it to tell apart different MSVC compiler versions.  But it 
is not present with MinGW.  So !_MSC_VER and !defined(_MSC_VER) have 
different meanings.  So in this particular case, more precision in the 
comments might be better.

On 29/08/2022 14:50, Peter Eisentraut wrote:
> I usually try to follow the guidelines in 
> <https://www.gnu.org/prep/standards/html_node/Comments.html>, which 
> pretty much match what you are proposing.

Thank you for the link, it's a useful one and the wording is better than 
mine.

> Note that for _MSC_VER in particular there is some trickiness: We 
> generally use it to tell apart different MSVC compiler versions.

That's certainly true in branches <= 15, but in master, to my surprise, 
I don't see any numerical comparisons of _MSC_VER since the recent 
6203583b7.

I'm not sure explicit !defined(_MSC_VER) is all that more clear
than !_MSC_VER in the commentary. After all, we would probably
never (?) see an actual
#if (!_MSC_VER)
in a real code.

So I have mixed feelings on forcing define() on _MSC_VER, but if you 
insist, I don't mind much either way.

What about other changes? Are there any obviously wrong or missed ones?

-- 
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru