Page MenuHomePhabricator

[sanitizer] Set IndentPPDirectives: AfterHash in .clang-format
ClosedPublic

Authored by MaskRay on Apr 9 2021, 5:45 PM.

Details

Summary

Code patterns like this are common, # at the line beginning
(https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives),
one space indentation for if/elif/else directives.

#if SANITIZER_LINUX
# if defined(__aarch64__)
# endif
#endif

However, currently clang-format wants to reformat the code to

#if SANITIZER_LINUX
#if defined(__aarch64__)
#endif
#endif

This significantly harms readability in my review. Use `IndentPPDirectives:
AfterHash` to defeat the diagnostic. clang-format will now suggest:

#if SANITIZER_LINUX
#  if defined(__aarch64__)
#  endif
#endif

Unfortunately there is no clang-format option using indent with 1 for
just preprocessor directives. However, this is still one step forward
from the current behavior.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 9 2021, 5:45 PM
MaskRay requested review of this revision.Apr 9 2021, 5:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 5:45 PM

I usually let clang-format to do default stuff and I would prefer we move complile-rt to style consistent with llvm and clang. I don't see we enabled indent for them, is it correct?

I usually let clang-format to do default stuff and I would prefer we move complile-rt to style consistent with llvm and clang. I don't see we enabled indent for them, is it correct?

Correct. We don't need preprocessor indentation for them because clang/llvm code bases don't tend to have nested #if. compiler-rt is different. Without indentation it is difficult to tell the scope of the change.

I am OK either way.
Let it wait few days and see if there is a stronger opinion.

vitalybuka accepted this revision.Apr 26 2021, 5:51 PM

As I said, I dislike inconsistency with llvm code to about the same degree as I like improvement in readability from the patch.
So LGTM if you still want this and there are no other objections.

This revision is now accepted and ready to land.Apr 26 2021, 5:51 PM

@euhlmann @krasimir How hard to support one-column indentation for preprocessor directives only? The majority of the sanitizer code base uses one-column indentation for nested directives.

@euhlmann @krasimir How hard to support one-column indentation for preprocessor directives only? The majority of the sanitizer code base uses one-column indentation for nested directives.

I too believe there's no knob to control preprocessor indentation independently of general code indentation. This could be quite tricky based on how long the set of patches to support one of the flavors of the IndentPPDirectives option (I believe that was an internship project). Preprocessor directive indentation is tricky since it may affect indentation around surrounding code constructs and comment blocks and it has code to deal with it at several layers of the formatting pipeline.

thakis added a subscriber: thakis.May 31 2022, 7:47 AM

Glancing through the output of (for f in $(find compiler-rt/lib -name '*.cpp' -o -name '*.h'); do sed -n '/#if /,/#endif/ p' $f | grep '#' ; done) | less, I have the impression that IndentPPDirectives: AfterHash is not the prevailing style in compiler-rt. So adding it causes clang-format failures on patches that are consistent with local style (…as has just happened to me at D126696, which is why I'm here).

Adding it has the addition disadvantage of making compiler-rt look less like other LLVM code.

I think it might be good to undo this change.

Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 7:47 AM

Glancing through the output of (for f in $(find compiler-rt/lib -name '*.cpp' -o -name '*.h'); do sed -n '/#if /,/#endif/ p' $f | grep '#' ; done) | less, I have the impression that IndentPPDirectives: AfterHash is not the prevailing style in compiler-rt. So adding it causes clang-format failures on patches that are consistent with local style (…as has just happened to me at D126696, which is why I'm here).

Indenting with one column is common, it's just that clang-format doesn't support one column for #.

Adding it has the addition disadvantage of making compiler-rt look less like other LLVM code.

lldb and compiler-rt were imported from different sources into llvm-project and inherently have some differences.
In some sense clang is different from LLVM code as many functions use UpperCase.

llvm/include/llvm/Support/Compiler.h uses indent-one after # scheme, which is quite similar to the style used in compiler-rt, at least the sanitizer part.

I think it might be good to undo this change.

Indenting with one column is common, it's just that clang-format doesn't support one column for #.

Did you look through the output of the command I pasted? As far as I can tell, indenting (by 1 or 2 or whatever) is much less common than not indenting. Am I looking at the wrong thing?

Indenting with one column is common, it's just that clang-format doesn't support one column for #.

Did you look through the output of the command I pasted? As far as I can tell, indenting (by 1 or 2 or whatever) is much less common than not indenting. Am I looking at the wrong thing?

I have read the output and I think # indentation is very common.

Indented # define vs un-indented #define:

% (for f in $(find compiler-rt/lib -name '*.cpp' -o -name '*.h'); do sed -n '/#if /,/#endif/ p' $f | grep '#' ; done) | awk '/#if/{s++} /#endif/{s--} s>0&&/# .*define/{c++} s>0&&/#define/{d++} END {print c"\t"d}'    
732     887