[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 528531.
Sedeniono added a comment.
Herald added subscribers: llvm-commits, pcwang-thead.
Herald added a project: LLVM.

Fixes github issues #59178, #58464 and #62799.

      

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the previous
indentation level. In case the --lines option was used
such that the previous deeper level was not formatted,
that previous level was whatever happened to be there
in the source code. The formatter simply believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> change in LevelIndentTracker::nextLine()).
Note that this used to be the case until LLVM 14.0.6,
but was changed in https://reviews.llvm.org/D129064
(#56352) to fix a crash. Our commit here essentially
reverts that crash fix. It was incorrect/incomplete.

Even with the revert, the crash from
https://reviews.llvm.org/D129064 (#56352) no longer
occurs, most likely because of the changes made in
https://reviews.llvm.org/D144296 (#60843).

The change in adjustToUnmodifiedLine() ensures that
the assert() is only checked if IndentForLevel is
actually accessed.

The new test FormatTestSelective.DontAssert checks
a case where a previous iteration of the present
patch triggered an assert().

The new tests in
FormatMacroRegardlessOfPreviousIndent check the
formatting of a preprocessor #define. They ensure
that the content of LevelIndentTracker::IndentForLevel
does not affect PP directives.

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
for some more details.

This commit is the 2nd try for
https://reviews.llvm.org/D151047


Herald added a comment.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an `NFC` or refactoring, adding documentation etc..)

Add your unit tests in `clang/unittests/Format` and you can build with `ninja FormatTests`.  We recommend using the `verifyFormat(xxx)` format of unit tests rather than `EXPECT_EQ` as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in `TokenAnnotatorTest.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  llvm/bindings/ocaml/llvm/META.llvm.in
  llvm/bindings/ocaml/llvm/llvm_ocaml.h


Index: llvm/bindings/ocaml/llvm/llvm_ocaml.h
===================================================================
--- llvm/bindings/ocaml/llvm/llvm_ocaml.h
+++ llvm/bindings/ocaml/llvm/llvm_ocaml.h
@@ -56,7 +56,6 @@
 #define Use_val(v) ((LLVMUseRef)from_val(v))
 #define BasicBlock_val(v) ((LLVMBasicBlockRef)from_val(v))
 #define MemoryBuffer_val(v) ((LLVMMemoryBufferRef)from_val(v))
-#define PassManager_val(v) ((LLVMPassManagerRef)from_val(v))
 
 /* Convert a C pointer to an OCaml option */
 value ptr_to_option(void *Ptr);
Index: llvm/bindings/ocaml/llvm/META.llvm.in
===================================================================
--- llvm/bindings/ocaml/llvm/META.llvm.in
+++ llvm/bindings/ocaml/llvm/META.llvm.in
@@ -37,14 +37,6 @@
     archive(native) = "llvm_executionengine.cmxa"
 )
 
-package "ipo" (
-    requires = "llvm"
-    version  = "@PACKAGE_VERSION@"
-    description = "IPO Transforms for LLVM"
-    archive(byte) = "llvm_ipo.cma"
-    archive(native) = "llvm_ipo.cmxa"
-)
-
 package "debuginfo" (
     requires = "llvm"
     version = "@PACKAGE_VERSION@"
@@ -61,14 +53,6 @@
     archive(native) = "llvm_irreader.cmxa"
 )
 
-package "scalar_opts" (
-    requires = "llvm"
-    version = "@PACKAGE_VERSION@"
-    description = "Scalar Transforms for LLVM"
-    archive(byte) = "llvm_scalar_opts.cma"
-    archive(native) = "llvm_scalar_opts.cmxa"
-)
-
 package "transform_utils" (
     requires = "llvm"
     version = "@PACKAGE_VERSION@"
@@ -77,22 +61,6 @@
     archive(native) = "llvm_transform_utils.cmxa"
 )
 
-package "vectorize" (
-    requires = "llvm"
-    version = "@PACKAGE_VERSION@"
-    description = "Vector Transforms for LLVM"
-    archive(byte) = "llvm_vectorize.cma"
-    archive(native) = "llvm_vectorize.cmxa"
-)
-
-package "passmgr_builder" (
-    requires = "llvm"
-    version = "@PACKAGE_VERSION@"
-    description = "Pass Manager Builder for LLVM"
-    archive(byte) = "llvm_passmgr_builder.cma"
-    archive(native) = "llvm_passmgr_builder.cmxa"
-)
-
 package "target" (
     requires = "llvm"
     version  = "@PACKAGE_VERSION@"


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151047.528531.patch
Type: text/x-patch
Size: 2081 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230605/5771e6eb/attachment.bin>