Poor readability of preprocessor generated by Tools/clinic

[rruuaanng]

Proposal:

# Add a code block here, if required

from #125958

The indentation of the preprocessor generated by clinic seems strange, it puts the preprocessor and normal statements in the same column. This makes it less readable, even if it is generated, which can maintain correctness, but it does not exclude that no developer will read the code.
for example:

cpython/Modules/_posixsubprocess.c

Lines 569 to 580 in 75401fe

	static void
	_close_open_fds(int start_fd, int *fds_to_keep, Py_ssize_t fds_to_keep_len)
	{
	#ifdef HAVE_ASYNC_SAFE_CLOSE_RANGE
	if (_close_range_except(
	start_fd, INT_MAX, fds_to_keep, fds_to_keep_len,
	_close_range_closer) == 0) {
	return;
	}
	#endif
	_close_open_fds_fallback(start_fd, fds_to_keep, fds_to_keep_len);
	}

This is easy to understand for indentation.

The following clinic generated code:

tatic PyObject *
cmath_acosh(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames)
{
    PyObject *return_value = NULL;
    #if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)

    #define NUM_KEYWORDS 1
    static struct {
        PyGC_Head _this_is_not_used;
        PyObject_VAR_HEAD
        PyObject *ob_item[NUM_KEYWORDS];
    } _kwtuple = {
        .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS)
        .ob_item = { _Py_LATIN1_CHR('z'), },
    };
    #undef NUM_KEYWORDS
    #define KWTUPLE (&_kwtuple.ob_base.ob_base)

    #else  // !Py_BUILD_CORE
    #  define KWTUPLE NULL
    #endif  // !Py_BUILD_CORE

    static const char * const _keywords[] = {"z", NULL};
    static _PyArg_Parser _parser = {
        .keywords = _keywords,
        .fname = "acosh",
        .kwtuple = KWTUPLE,
    };
    #undef KWTUPLE
    PyObject *argsbuf[1];
    Py_complex z;
    Py_complex _return_value;

    args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, argsbuf);
    if (!args) {
        goto exit;
    }
    z = PyComplex_AsCComplex(args[0]);
    if (PyErr_Occurred()) {
        goto exit;
    }
@@ -93,25 +153,55 @@ cmath_acosh(PyObject *module, PyObject *arg)
}

Forgive me for not seeing the preprocessor at first glance. If it was not designed with reading in mind, I will close this PR.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

[ericvsmith]

I think you should use a C formatter (aka pretty-printer) if you want to see the code formatted differently. But I'm not sure they'd format #ifdefs the way you'd like. Note that in your example, the code is indented not because of the #ifdef but because of the brace on the preceding line.

[skirpichev]

I think you should use a C formatter (aka pretty-printer) if you want to see the code formatted differently.

Probably, it's a good idea and simple to implement (c.f. manual code formatting in the Tools/clinic).

But I'm not sure they'd format #ifdefs the way you'd like.

The indent do this (--preprocessor-indentation3, for example). Though, I don't sure which style accurately follows PEP 7.

[ZeroIntensity]

PEP-7 doesn't specify what to do for processor macros, I think. It might be worth documenting it. FWIW, I like the current style though.

[rruuaanng]

I think you should use a C formatter (aka pretty-printer) if you want to see the code formatted differently. But I'm not sure they'd format #ifdefs the way you'd like. Note that in your example, the code is indented not because of the #ifdef but because of the brace on the preceding line.

Actually, what I'm trying to say is that the preprocessor directives should be aligned in the first column, not indented with the statements.

void f()
{
#if 1
    if()
        ....
    else
        ....
    /* other */
#endif
}

void f()
{
    #if 1
    if()
        ....
    else
        ....
    /* other */
    #endif
}

[skirpichev]

I like the current style though.

Which one? Sometimes there are no indentation for nested preprocessor directives, sometimes indentation with one space, sometimes two.

what I'm trying to say is that the preprocessor directives should be aligned in the first column

That too. This part seems to be consistent across hand-made files.

[rruuaanng]

That too. This part seems to be consistent across hand-made files.

Yes, that's what I want to say. The generated code is inconsistent with the style of manual writing, which is actually strange. And from my example, it's very simple. What if the situation is complicated?

[rruuaanng]

I think we should specify the style of the preprocessor in PEP7, which is really common. And it is difficult to read. If it is not strictly stipulated, it will greatly reduce the readability.

For example
Good

...
#if 1
# define A
#else
# define B
#endif // 1

Bad

...
# if 1
#  define A
#else
#  define B
#endif
...

If the above example is complicated, how many spaces are needed? And how should it end?

[ZeroIntensity]

Honestly, PEP 7 probably needs to get some updates. There's plenty we don't specify, at least compared to PEP 8.

Actually, what I'm trying to say is that the preprocessor directives should be aligned in the first column, not indented with the statements.

Aren't we already doing that in most code, and what clinic is doing already?

[rruuaanng]

Aren't we already doing that in most code, and what clinic is doing already?

In fact, the code generated by the clinic is not consistent with the code style written by humans, which I have given above.

If my request is allowed, I would like to apply for PEP7 to add specifications on preprocessing style. Even if we cannot refactor the existing clinic-generated code on a large scale, we must stipulate the style of manual writing.

[ZeroIntensity]

Ah, I do see that clinic outputs that style. Generated code doesn't have a readability requirement though, so I would be hesitant to change things solely for that purpose.

Regarding PEP 7 changes, I highly suggest going to DPO first, because nobody can agree on styles. I'm almost certain there are core devs who like indenting the preprocessor directives.

[picnixz]

For AC, I personally think we shouldn't strive for "readability". Heck, I'd even be in favor of having compressed files in AC just for the sake of minimizing the repository size but AC has other issues that are more pressing IMO. If we were to make it PEP-7 compliant, we'll have a very huge diff which I think is not worth since it could create conflicts on existing PRs (and probably many people will need to re-run AC with a forced flag or just remove all clinic files).

[rruuaanng]

I'm not referring to the files generated by the clinic, but the overall style of the code base.

Which one? Sometimes there are no indentation for nested preprocessor directives, sometimes indentation with one space, sometimes two.

I noticed this problem a long time ago. In fact, the indentation of preprocessing in many places is not uniform.

Edit:
I think PEP7 should probably describe the rules for preprocessor directive indentation for future development. Even if we don't change the clinic tool, it is necessary to specify the preprocessor format.

[picnixz]

I'm not referring to the files generated by the clinic, but the overall style of the code base.

If you want to specify a macro guide for new code, you should open a DPO and follow PEP-I-don't-remember-the-number to update PEP-7 (there should be a core dev + SC vote IIRC or something like that). I think new pre-processing directives are usually following the file style or author's common sense.

Now, if you are not referring to AC-files, then this issue should be closed (or the title should be changed and labels should be updated).

---

In general, if I have nested #if, I indent using spaces after # but not before it:

#if defined(X)
#  if defined(Y)
#  endif 
#endif

I also put # at the first column because otherwise my macros cannot be too wide :')

[rruuaanng]

This issue is mainly about clinic. Actually, I just question whether the generated code should be consistent with the manual code.

#if defined(X)
#  if defined(Y)
#  endif 
#endif

In fact, the code you gave is different from the style in the repository. On the first layer of preprocessing it should only add one space, but you are adding two.

It should be like this

#if defined(X)
# if defined(Y)
#  if defined(Z)
#  endif
# endif 
#endif

Edit:
For the code you gave, there is also the code I gave. They are actually both correct, but when a large number of users contribute, this leads to a lot of pre-processed styles in the code base. This means that if the guidelines I gave were applied to the clinic, there would be a lot of code that does not conform to the style.

[picnixz]

This issue is mainly about clinic

If it's about clinic, then my stance is: we don't change the style. You are contradicting yourself with "I'm not referring to the files generated by the clinic, but the overall style of the code base.".

In fact, the code you gave is different from the style in the repository. On the first layer of preprocessing it should only add one space, but you are adding two.

Not really. There are only 16 occurrences of # if and 85 of # if (two spaces). Just have a look at pycore_import.h or pyport.h. For # ifdef there are 13 occurrences vs 107 for # ifdef (agreed that there are probably nested ones but just the fact that we don't use single spaces a lot is an indication).

[rruuaanng]

There seems to be a misunderstanding. The style I refer to includes both clinic and human-written styles. But leaving human-written styles aside, the clinic-generated codes given above make it confusing for users who try to read them. like this

void f()
{
    #if 1
    if()
        ....
    else
        ....
    /* other */
    #endif
}

Edit:
To be honest, when I first saw it, I didn't notice that it had any pre-processing.

[skirpichev]

For AC, I personally think we shouldn't strive for "readability".

The immanent property of any auto-generated code is that sometimes people read one. E.g. to debug things.

Heck, I'd even be in favor of having compressed files in AC just for the sake of minimizing the repository size but AC has other issues that are more pressing IMO.

There are reasons to include such auto-generated code in the repository history. Obvious one is written right above;)

But I would speculate, that with using some indent-like autoformatter - clinic output will be more predictable.

If we were to make it PEP-7 compliant, we'll have a very huge diff

That's a good point, but I'm not sure if this is so severe issue. Conflicts for autogenerated files are easy to fix. Just regenerate them.

Now, if you are not referring to AC-files, then this issue should be closed

I think this is still about AC. But to address that issue - PEP 7 should be updated first.

PS: for workflow, see e.g. python/peps#3516. I don't think we should bother SC;)

[rruuaanng]

The immanent property of any auto-generated code is that sometimes people read one. E.g. to debug things.

Yes, the user may not notice the existence of a preprocessor during debugging due to the same indentation.
Moreover, the clinic style and project style mentioned in this issue are not contradictory. In fact, they refer to the same content.

[erlend-aasland]

Generated code does not need to follow PEP-7. Moreover, we never do stylistic changes like this; we follow PEP-7 when we add new code, and we sometimes adapt exiting code to PEP-7 if we change it (for example in a bugfix).

Thanks for the suggestion, but I think we'll pass on this.