Thread: Allow declaration after statement and reformat code to use it

## What is this?

It's a draft patch that replaces code like this:

```c

pg_file_unlink(PG_FUNCTION_ARGS)

{

char   *filename;

requireSuperuser();

filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));

```

With this shorter version:

```c

pg_file_unlink(PG_FUNCTION_ARGS)

{

requireSuperuser();

char      *filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));

```

## Why would we want this?

1. It removes 23328 lines of code that don't have any impact on how the code behaves [1]. This is roughly 2.7% of all the lines of code in the codebase.

2. Declarations are closer to the actual usage. This is advised by the "Code Complete" book [2] and has the following advantages:

   a. This limits variable scope to what is necessary. Which in turn makes the mental model you have to keep of a function when reading the code simpler.

   b. In most cases it allows you to see the the type of a variable without needing to go to the top of the function.

3. You can do input checking and assertions at the top of the function, instead of having to put declarations in front of it. This makes it clear which invariants hold for the function.  (as seen in the example above and the changes for pg_file_rename[3])

4. Declaring variables after statements is allowed in C99. Postgres already requires C99, so it might as well use this feature too.

## How was this changeset created?

I created a script that modifies all C files in the following way:

1. Find a non `static` declaration.

2. If it has an initializer and it is not a single variable/constant, don't consider replacing it. (reason: a function call initializer might have sideffects).

3. Otherwise (it's a simple initializer or it has no initializer at all), take the type and variable from that declaration.

4. Find the next use of the variable.

5. If the next use is not an assignment, don't do anything (the value of the original initialization is used).

6. If the next use is an assignment:

    1. Remove the old declaration

    2. Prefix the found assignment with the type

    3. Unless the variable is also used in the same line of the new initialization, e.g:

      ```c

      int a = 1;

      a = a + a;

      ```

## How does this script work?

It uses a Perl regex to search and replace! (obligatory jokes at the bottom of the email) This regex is based on the ones used in this PR to citus[4] and the similar PR to pg_auto_failover[5]. The script is in the 3rd commit of this patchset.

To visualize the regex in the script in a reasonable way, copy paste the search part of it:

```

\n\t(?!(return|static)\b)(?P<type>(\w+[\t ])+[\t *]*)(?>(?P<variable>\w+)( = [\w>\s\n-]*?)?;\n(?P<code_between>(?>(?P<comment_or_string_or_not_preprocessor>\/\*.*?\*\/|"(?>\\"|.)*?"|(?!goto)[^#]))*?)(\t)?(?=\b(?P=variable)\b))(?<=\n\t)(?<!:\n\t)(?P=variable) =(?![^;]*?[^>_]\b(?P=variable)\b[^_])

```

And paste that into https://www.debuggex.com/, then select PCRE from the dropdown. (Sharing seems to be down at this point, so this is the only way to do it at the moment) Try it out! The regex is not as crazy as it looks.

There's two important assumptions this regex makes:

1. Code is indented using tabs, and the intent level determines the scope. (this is true, because of `pgindent`)

2. Declared variables are actually used. (this is true, because we would get compiler warnings otherwise)

There's two cases where this regex has some special behaviour:

1. Stop searching for the first usage of a variable when either a `goto` or a preprocessor command is found (outside a string or comment). These things affect the control flow in a way that the regex does not understand. (any `#` character is assumed to be a preprocessor commands).

2. Don't replace if the assignment is right after a `label:`, by checking if there was a `:` as the last character on the previous line. This works around errors like this:

```

hashovfl.c:865:3: error: a label can only be part of a statement and a declaration is not a statement

   OffsetNumber maxroffnum = PageGetMaxOffsetNumber(rpage);

   ^~~~~~~~~~~~

```

Detecting this case in this way is not perfect, because sometimes there is an extra newline or a comment between the label and the assignment. This is not easily detectable by the regex, because lookbehinds cannot have a variable length in Perl (or most regex engines for that matter). For these few cases (5 in the entire codebase) a manual change was done either before or after the automatic replacement to fix them so the code compiles again. (2nd and 5th commits of this patchset)

After all these changes `make -s world` doesn't show any warnings or errors and `make check-world` succeeds. I configured compilation like this:

```

./configure --enable-cassert --enable-depend --enable-debug --with-openssl --with-libxml --with-libxslt --with-uuid=e2fs --with-icu

```

## What I want to achieve with this email

1. For people to look at a small sample of the changes made by this script. I will try to send patches with the actual changes made to the code as attachments in a followup email. However, I don't know if that will succeed since the patch files are very big and it might run into some mailserver limits. So in case that doesn't work, the full changeset is available on Github[6]. This make viewing this enormous diff quite a bit easier IMHO anyaway. If you see something weird that is not covered in the "Known issues" section below, please share it. That way it can be discussed and/or fixed.

2. A discussion on if this type of code change would be a net positive for Postgres codebase. Please explain clearly why or why not.

3. Some links to resources on what's necessary to get a big refactoring patch like this merged.

## What I don't want to achieve with this email

1. For someone to go over all the changes right now. There's likely to be changes to the script or something else. Doing a full review of the changes would be better saved for later during a final review.

## Known issues with the currently generated code

There's a few issues with the final generated code that I've already spotted. These should all be relatively easy to fix in an automated way. However, I think doing so is probably better done by `pgindent` or some other auto formatting tool, instead of with the regex. Note that I did run `pgindent`, it just didn't address these things:

1. Whitespace between type and variable is kept the same as it was before moving the declaration. If the original declaration had whitespace added in such a way that all variable names of declarations aligned, then this whitespace is preserved. This looks weird in various cases. See [3] for an example.

2. `pgindent` adds a newline after each block of declarations, even if they are not at the start of function. If this is desired is debatable, but to me it seems like it adds excessive newlines. See [7] for an example.

3. If all declarations are moved away from the top of the function, then an empty newline is kept at the top of the function. This seems like an unnecessary newline to me. See [3] for an example.

Sources:

1. `tokei`[8] results of lines of code:

Before:

```

$ tokei --type C

===============================================================================

 Language            Files        Lines         Code     Comments       Blanks

===============================================================================

 C                    1389      1361309       866514       330933       163862

===============================================================================

 Total                1389      1361309       866514       330933       163862

===============================================================================

```

After:

```

$ tokei --type C

===============================================================================

 Language            Files        Lines         Code     Comments       Blanks

===============================================================================

 C                    1389      1347515       843186       330952       173377

===============================================================================

 Total                1389      1347515       843186       330952       173377

===============================================================================

```

2. https://github.com/mgp/book-notes/blob/master/code-complete.markdown#103-guidelines-for-initializing-variables

3.

```patch

@@ -401,11 +389,10 @@ pg_file_rename_internal(text *file1, text *file2, text *file3)

 Datum

 pg_file_unlink(PG_FUNCTION_ARGS)

 {

- char   *filename;

 

  requireSuperuser();

 

- filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));

+ char   *filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));

 

  if (access(filename, W_OK) < 0)

```

4. https://github.com/citusdata/citus/pull/3181

5. https://github.com/citusdata/pg_auto_failover/pull/266

6. https://github.com/JelteF/postgres/pull/2

7.

```patch

@@ -2863,7 +2886,8 @@ palloc_btree_page(BtreeCheckState *state, BlockNumber blocknum)

  * longer than we must.

  */

  Buffer buffer = ReadBufferExtended(state->rel, MAIN_FORKNUM, blocknum, RBM_NORMAL,

- state->checkstrategy);

+ state->checkstrategy);

+

  LockBuffer(buffer, BT_READ);

 

  /*

```

8. https://github.com/XAMPPRocky/tokei

Obligatory jokes:

1. https://xkcd.com/1171/

2. https://xkcd.com/208/

3. https://stackoverflow.com/a/1732454/2570866 (and serious response to it https://neilmadden.blog/2019/02/24/why-you-really-can-parse-html-and-anything-else-with-regular-expressions/)

However, even if such an idea were to get the green light, I think I would

take the obligatory regex jokes seriously, and instead use something like

srcML [0] and do the analysis and modification on proper parse trees.

@Chapman I think that's a reasonable ask. This is the first time I heard of srcML. It indeed looks like it would be a much better tool than a regex to do this refactor. The refactor logic would still be roughly the same though because the regex essentially extracts and modifies a parse tree too. Just one that only has the bare minimum requirements for this specific refactor. I would like to state, that we've used this regex to refactor both Citus and pg_auto_failover, and no bugs turned up because of it so far. This is probably, because a regex is such a horrible way of writing a parser in the first place. So, any bugs in the regex will very likely create code that does not compile at all, instead of code that can be compiled but has some obscure bug .

I think effectively it would need to be run on all supported versions,

which means the risk of introducing errors would have to be very low.

@Bruce @Tom @Chapman I agree that doing this refactor only on the current master branch would cause way more pain due to backpatching, than the amount of happiness it provides by having easier to reason about code. I like the idea of avoiding this backpatching problem by running it . Luckily, running the automatic refactor on all branches should be pretty easy. However, it seems like PG11 and below only require a C89 compatible compiler. Am I correct in assuming increasing that requirement to C99 is not really an option? If so, I think it might be best if I revive this thread in ~27 months when PG11 is not supported anymore.

The rule against declaration-after-statement was kept
after significant discussion, which you can find in the archives if you

look. 

@Tom I guess my search skills in the archives are not very good, because I cannot find any big discussion on about declaration-after-statement. The only discussion I can find is this: https://www.postgresql.org/message-id/877efaol77.fsf@news-spur.riddles.org.uk If you're able to find that thread, could you share it?

C needs readability, not fewer lines.

Aside from horrible code, it doesn't improve 0.1% on anything.

@Ranier I tried to explain in my initial email that it not only reduces lines, but more importantly improves readability quite a bit IMHO:

2. Declarations are closer to the actual usage. This is advised by the "Code Complete" book [2] and has the following advantages:

   a. This limits variable scope to what is necessary. Which in turn makes the mental model you have to keep of a function when reading the code simpler.

   b. In most cases it allows you to see the the type of a variable without needing to go to the top of the function.

3. You can do input checking and assertions at the top of the function, instead of having to put declarations in front of it. This makes it clear which invariants hold for the function.  (as seen in the example above and the changes for pg_file_rename[3])

If you disagree with those statements, could you explain why?

On 8/20/21 12:30 PM, Ranier Vilela wrote:
>
>
> There is a reason why GMs Brian Kernighan and Dennis Ritchie made the
> C89, less buggy.
> IMHO C99 makes it easy to make more mistakes.
> One more step and we won't even need to declare a variable.
>
>

I've used both styles in different languages over the years. I don't
know that one is a clearly better way, notwithstanding what various
theorists might say.

Note that in C89 it's fantastically easy to put the declaration as close
as you like to the first use of a variable. All it takes is a pair of
braces enclosing the variable scope.

Even if we were to relax our rules on this, making wholesale changes
along these lines and even more backpatching them seem out of the question.