[PATCH] D153243: [clang-format] Don't finalize #if, #else, #endif, etc.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1416-1421
+      if (Tok->is(tok::hash) && !Tok->Previous && Tok->Next &&
+          Tok->Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_ifndef,
+                             tok::pp_elif, tok::pp_elifdef, tok::pp_elifndef,
+                             tok::pp_else, tok::pp_endif)) {
+        Tok = Tok->Next;
+      }
----------------
Sedeniono wrote:
> After some time, I found your comment [here](https://reviews.llvm.org/D150057#inline-1449546) which explains the intention of the "do not finalize '#'" thing. Would it be possible to provide the reasoning here as some comment, or at least some hint that it has to do with multiple passes because of PP branches? To give future readers some chance to understand what is going on. Maybe something along the lines of
> ```
> // Never mark the '#' of a PP branch as finalized, since clang-format performs 
> // multiple passes, where each pass represents a version where the code of 
> // one of the PP branches is filled in. Later passes must also  be allowed to 
> // format the PP tokens.
> ```
> Alternatively or additionally, I would be happy with a link to [your explanation](https://reviews.llvm.org/D150057#inline-1449546) in the code or the commit message, if the llvm rules/conventions allow it.
> 
> Apart from this, I have dug way too little into how clang-format works to reason about the change. I tested it with the full hpp file from https://github.com/llvm/llvm-project/issues/57117, and can confirm that the issue is fixed. So I believe you. ;-)
> But: Considering your comment [here](https://reviews.llvm.org/D150057#inline-1449546), I thought that the whole "do not finalize '#'" thing is relevant only if there are some "else"-PP-branches? But in the minimal example/the unit test, there is no "else" branch? How do the multiple passes because of PP branches relate to the `clang-format off` comment?
> Alternatively or additionally, I would be happy with a link to [your explanation](https://reviews.llvm.org/D150057#inline-1449546) in the code or the commit message, if the llvm rules/conventions allow it.

I've updated the summary and will include the link in the commit message.

> But: Considering your comment [here](https://reviews.llvm.org/D150057#inline-1449546), I thought that the whole "do not finalize '#'" thing is relevant only if there are some "else"-PP-branches? But in the minimal example/the unit test, there is no "else" branch?

This patch fixes the issue that `#if`/`#else` etc. may get formatted even if `clang-format off` is specified. It's not specific to `#else`, and the added unit test is sufficient.

> How do the multiple passes because of PP branches relate to the `clang-format off` comment?

They are not (or should not be) related. We probably should add an attribute to `FormatToken` exclusively for `clang-format off` as `Finalized` doesn't necessarily mean `clang-format off` is in effect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153243/new/

https://reviews.llvm.org/D153243