Prevent first value of header guards from being aligned (and more generally not be re-formatted)

[Julien-Elie]

When AcrossEmptyLinesAndComments is used for AlignConsecutiveMacros, alignment is done like this:

#ifndef INN_PATHS_H
#define INN_PATHS_H                1

/* comment */

#define INN_PATH_NNRPD             "nnrpd"
#define INN_PATH_AUTHDIR_NOPASS    "resolv"

Couldn't the value of the first line of the header guard not be aligned?

[mydeveloperday]

out of interest most header guards are written as

#ifndef LLVM_CLANG_LIB_FORMAT_FORMATTOKEN_H
#define LLVM_CLANG_LIB_FORMAT_FORMATTOKEN_H

why the need for 1?

[Julien-Elie]

It is true that 1 is not necessary for header guards, that's a good point.

I've just been accustomed to always defining macros to something, which can then be tested in #if clauses. But you're right that I'm not going to test header guards later in the code.
That's a minor formatting nit though, so I'll consider removing the 1 in my header guards. But maybe I am not the only one to add 1 in similar definitions, and therefore it could be worthwhile preventing clang-format from reformatting it? (I'm of course OK if you believe it should still be.)

[mydeveloperday]

To be honest I slightly feel that if we were going to do this we'd have to add yet another option, I'm not convinced this is worth it, I mean you set it to AcrossCommentsEmptyLines and that's what you got. Lets add it as an enhancement for now but as you have a workaround My feeling is it will just sit here not getting done (unless you'd like to contibute a fix yourself)

[Julien-Elie]

Why not just make header guards special and do not apply them any formatting? (like PPIndentWidth does not apply)

[mydeveloperday]

If I understand correctly PPIndentWidth possibly does not apply because its at indent 0 because its first. (so its not really not applying it, its just not indenting)

I don't think to my knowledge that we assume anything about header guards, I've marked it as an enhancement, but honestly I can't really see it being a priority (at least for me). I just want to be clear of the expectation, That said you are free to submit patches if this is painful to you. (but it will need to be peer reviewed)

I can't see it just being something that is blanket everyone wouldn't want it to align. AlignConsecutiveMacros was a much sought after feature, its been in for a couple of years and this is the first time I've seen this raised. What ever you do it will have to be another option, (IgnoreHeaderGuardFormatting?) to ensure you don't break other people.

My advice honestly, is to change your style when it comes to header guards, I know that seems super harsh but there is so much code out there like the following that people WOULD want Aligned that I fear it would be way too easy for us to miss interpret an #ifndef as a header guard, I'm not sure I'd trust any code to be able to disambiguate between the two 100% of the time. (especially in the absence of real header guards)

#ifndef EAFNOSUPPORT
#define EAFNOSUPPORT 9901
#endif

[Julien-Elie]

What is new is the AcrossEmptyLinesAndComments feature for AlignConsecutiveMacros. It is how I saw the formatting change in header guards. They are usually separated by an empty line with the rest of the code, so AlignConsecutiveMacros did not affect them before.

I think the example for EAFNOSUPPORT will not be considered by clang-format as a header guard because the #endif will not be at the end of the file. So it will be properly indented.

Header guards are explicitly excluded from indentation, and are handled by clang-format (I've for instance seen https://reviews.llvm.org/D42035).

[mydeveloperday]

That's a very good point, definitely we should consider it and that's good evidence as to why we might. To be honest most changes like this need someone to champion them.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue