[PATCH] D35955: clang-format: Add preprocessor directive indentation

This revision was automatically updated to reflect the committed changes. Closed by commit rL312125: clang-format: Add preprocessor directive indentation (authored by krasimir). Changed prior to commit: https://reviews.llvm.org/D35955?vs=112432=113261#toc Repository: rL LLVM

[PATCH] D35955: clang-format: Add preprocessor directive indentation

krasimir added a comment. I'll land this for you, as discussed offline. The best thing is to apply for commit rights after you have a few patches landed. https://reviews.llvm.org/D35955

[PATCH] D35955: clang-format: Add preprocessor directive indentation

krasimir added a comment. @euhlmann: are you planning to commit this? https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added a comment. I'm glad this is finally in a state to land. Thanks for the helpful reviews! https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35955: clang-format: Add preprocessor directive indentation

krasimir added a comment. Thank you! I understand this patch better now. Looks good from my side! https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. From my side this looks good for now (we can always improve more later). Krasimir, what do you think? https://reviews.llvm.org/D35955 ___

[PATCH] D35955: clang-format: Add preprocessor directive indentation

mzeren-vmw added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:383 + Current.Previous->is(tok::hash) && State.FirstIndent > 0) { +// subtract 1 so indent lines up with non-preprocessor code +Spaces += State.FirstIndent; djasper

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:737 +for (auto& Line : Lines) { + if (Line.InPPDirective && Line.Level > 0) +--Line.Level; krasimir wrote: > Wouldn't this also indent lines continuing macro

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann updated this revision to Diff 112432. euhlmann marked 7 inline comments as done. euhlmann added a comment. - Swapped checks in ContinuationIndenter - Renamed `PPMaybeIncludeGuard` to `IfNdefCondition` and added comment - Added another bool member that tracks if include guard detection

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: unittests/Format/FormatTest.cpp:2297 + "#include \n" + "#define MACRO " + "\\\n" Please just set

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:387 +// hash. This causes second-level indents onward to have an extra space +// after the tabs. We set the state to column 0 to avoid this misalignment. +if (Style.UseTab !=

[PATCH] D35955: clang-format: Add preprocessor directive indentation

krasimir added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:383 + State.Line->Type == LT_ImportStatement) && + Current.Previous->is(tok::hash) && State.FirstIndent > 0) { +Spaces += State.FirstIndent; You can replace

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added a comment. Krasimir: Can you actually give this a round of review? I will also try to do so tomorrow. https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann updated this revision to Diff 111606. euhlmann marked an inline comment as done. euhlmann edited the summary of this revision. euhlmann added a comment. Eliminated redundancy in tests by fixing test::messUp which was incorrectly merging lines. Fixing the issue Mark brought up regarding

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann updated this revision to Diff 111433. euhlmann marked 13 inline comments as done. euhlmann edited the summary of this revision. euhlmann added a comment. Allows comments before the include guard opens. However, if there's a single non-comment line before an include-guard-like structure

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: unittests/Format/FormatTest.cpp:2281 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) { verifyFormat("#define A \\\n" mzeren-vmw wrote: > mzeren-vmw wrote: > > Experimenting with the patch

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added inline comments. Comment at: unittests/Format/FormatTest.cpp:2405-2408 + // Defect: We currently do not deal with cases where legitimate lines may be + // outside an include guard. Examples are #pragma once and + // #pragma GCC diagnostic, or anything else that

[PATCH] D35955: clang-format: Add preprocessor directive indentation

mzeren-vmw added inline comments. Comment at: unittests/Format/FormatTest.cpp:2281 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) { verifyFormat("#define A \\\n" mzeren-vmw wrote: > Experimenting with the patch locally I found that

[PATCH] D35955: clang-format: Add preprocessor directive indentation

mzeren-vmw added a comment. Daniel's right. We need need substantially more tests! Comment at: docs/ClangFormatStyleOptions.rst:1199 + * ``PPDIS_AfterHash`` (in configuration: ``AfterHash``) +Indents directives after the hash, counting the hash as a column. +

[PATCH] D35955: clang-format: Add preprocessor directive indentation

klimek added a comment. LG from my side. https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann updated this revision to Diff 110602. euhlmann edited the summary of this revision. euhlmann added a comment. The patch now uses `PPBranchLevel` to track indent level. It allows `PPBranchLevel` to go back down to -1. The existing `PPBranchLevel >= 0` checks appear to prevent indexing

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann updated this revision to Diff 110429. euhlmann edited the summary of this revision. euhlmann added a comment. This addresses Daniel's previous concerns. The option is now an enum, the heuristic for detecting include guards is expanded and has corresponding unit tests, and style issues

[PATCH] D35955: clang-format: Add preprocessor directive indentation

klimek added a comment. In https://reviews.llvm.org/D35955#835994, @euhlmann wrote: > In https://reviews.llvm.org/D35955#835439, @klimek wrote: > > > I think if we need this info, we can just make it count down to -1 again > > (or, but that's isomorphic, let it run from 0 and make sure we never

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added a comment. In https://reviews.llvm.org/D35955#835439, @klimek wrote: > I think if we need this info, we can just make it count down to -1 again (or, > but that's isomorphic, let it run from 0 and make sure we never index into > the data structures at 0 :) Should I do one of

[PATCH] D35955: clang-format: Add preprocessor directive indentation

klimek added a comment. In https://reviews.llvm.org/D35955#834914, @djasper wrote: > Manuel: Can you take a look at the last comment here? Why does PPBranchLevel > start at -1? It's a perhaps too-clever implementation to make sure that we can have a per-branch data structure (indexed from 0)

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added a comment. Manuel: Can you take a look at the last comment here? Why does PPBranchLevel start at -1? https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added inline comments. Comment at: lib/Format/UnwrappedLineParser.h:238 + unsigned PPIndentLevel; + FormatToken *PPMaybeIncludeGuard; djasper wrote: > I think this should almost always be PPBranchLevel. Probably the different > case is the #else

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:383 + Current.Previous->is(tok::hash) && State.FirstIndent > 0) { +// subtract 1 so indent lines up with non-preprocessor code +Spaces += State.FirstIndent; euhlmann

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:701 + PPMaybeIncludeGuard->TokenText == FormatTok->TokenText && + PPIndentLevel > 0) { +--PPIndentLevel; djasper wrote: > I think you'll need substantially more tests

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:383 + Current.Previous->is(tok::hash) && State.FirstIndent > 0) { +// subtract 1 so indent lines up with non-preprocessor code +Spaces += State.FirstIndent; djasper

[PATCH] D35955: clang-format: Add preprocessor directive indentation

djasper added inline comments. Comment at: docs/ClangFormatStyleOptions.rst:1182 +**IndentPPDirectives** (``bool``) + Indent preprocessor directives on conditionals. I think we can foresee that a bool is not going to be enough here. Make this an enum, which

[PATCH] D35955: clang-format: Add preprocessor directive indentation

euhlmann created this revision. euhlmann added a project: clang. This is an implementation for bug 17362 which adds support for indenting preprocessor statements inside if/ifdef/endif. This takes previous work from fmauch and makes it into a