[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D140058#4000332 , @rupprecht wrote:

> In D140058#4000311 , @rupprecht 
> wrote:
>
>> This is causing a test failure: 
>> https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
>> (and likewise running `ninja check-clang-unit` locally, but I don't have a 
>> buildbot link to that)
>
> Fixed in 5a06334c51aa75d7f044785a495cf2de5bf13a9c 
> 

Thanks!




Comment at: clang/lib/Format/Format.cpp:3408
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });

rupprecht wrote:
> This looks like use-after-free. S is captured by reference but the body of 
> the lambda executes after this scope block has ended.
> 
> BracesInserter itself takes it by ref but then stores a copy, but by the time 
> the constructor runs, the reference is already dangling.
My bad!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140058

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D140058#4000311 , @rupprecht wrote:

> This is causing a test failure: 
> https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
> (and likewise running `ninja check-clang-unit` locally, but I don't have a 
> buildbot link to that)

Fixed in 5a06334c51aa75d7f044785a495cf2de5bf13a9c 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140058

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

This is causing a test failure: 
https://buildkite.com/llvm-project/upstream-bazel/builds/48607#0185190c-43f8-43ff-b8bd-fa8ce0b6e2f5
(and likewise running `ninja check-clang-unit` locally, but I don't have a 
buildbot link to that)




Comment at: clang/lib/Format/Format.cpp:3408
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });

This looks like use-after-free. S is captured by reference but the body of the 
lambda executes after this scope block has ended.

BracesInserter itself takes it by ref but then stores a copy, but by the time 
the constructor runs, the reference is already dangling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140058

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Owen Pan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG240e29c5015d: [clang-format][NFC] Turn on some code-changing 
options one by one (authored by owenpan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140058

Files:
  clang/lib/Format/Format.cpp


Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1907,9 +1907,7 @@
 class BracesInserter : public TokenAnalyzer {
 public:
   BracesInserter(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.RemoveBracesLLVM = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -1962,9 +1960,7 @@
 class BracesRemover : public TokenAnalyzer {
 public:
   BracesRemover(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.InsertBraces = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -3347,6 +3343,9 @@
   FormatStyle Expanded = Style;
   expandPresetsBraceWrapping(Expanded);
   expandPresetsSpaceBeforeParens(Expanded);
+  Expanded.InsertBraces = false;
+  Expanded.RemoveBracesLLVM = false;
+  Expanded.RemoveSemicolon = false;
   switch (Expanded.RequiresClausePosition) {
   case FormatStyle::RCPS_SingleLine:
   case FormatStyle::RCPS_WithPreceding:
@@ -3403,20 +3402,26 @@
 }
 
 if (Style.InsertBraces) {
+  FormatStyle S = Expanded;
+  S.InsertBraces = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveBracesLLVM) {
+  FormatStyle S = Expanded;
+  S.RemoveBracesLLVM = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveSemicolon) {
+  FormatStyle S = Expanded;
+  S.RemoveSemicolon = true;
   Passes.emplace_back([&](const Environment ) {
-return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return SemiRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 


Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1907,9 +1907,7 @@
 class BracesInserter : public TokenAnalyzer {
 public:
   BracesInserter(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.RemoveBracesLLVM = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -1962,9 +1960,7 @@
 class BracesRemover : public TokenAnalyzer {
 public:
   BracesRemover(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.InsertBraces = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -3347,6 +3343,9 @@
   FormatStyle Expanded = Style;
   expandPresetsBraceWrapping(Expanded);
   expandPresetsSpaceBeforeParens(Expanded);
+  Expanded.InsertBraces = false;
+  Expanded.RemoveBracesLLVM = false;
+  Expanded.RemoveSemicolon = false;
   switch (Expanded.RequiresClausePosition) {
   case FormatStyle::RCPS_SingleLine:
   case FormatStyle::RCPS_WithPreceding:
@@ -3403,20 +3402,26 @@
 }
 
 if (Style.InsertBraces) {
+  FormatStyle S = Expanded;
+  S.InsertBraces = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveBracesLLVM) {
+  FormatStyle S = Expanded;
+  S.RemoveBracesLLVM = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveSemicolon) {
+  FormatStyle S = Expanded;
+  S.RemoveSemicolon = true;
   Passes.emplace_back([&](const Environment ) {
-return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return SemiRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140058: [clang-format][NFC] Turn on some code-changing options one by one

[Owen Pan via Phabricator via cfe-commits]

owenpan created this revision.
owenpan added reviewers: MyDeveloperDay, HazardyKnusperkeks, rymiel.
owenpan added a project: clang-format.
Herald added a project: All.
owenpan requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For the code-changing options `InsertBraces`, `RemoveBracesLLVM`, and 
`RemoveSemicolon`, turn the option on only when running the token analyzer pass 
for it. This improves the run-time and avoids interference from other options.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140058

Files:
  clang/lib/Format/Format.cpp


Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1907,9 +1907,7 @@
 class BracesInserter : public TokenAnalyzer {
 public:
   BracesInserter(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.RemoveBracesLLVM = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -1962,9 +1960,7 @@
 class BracesRemover : public TokenAnalyzer {
 public:
   BracesRemover(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.InsertBraces = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -3347,6 +3343,9 @@
   FormatStyle Expanded = Style;
   expandPresetsBraceWrapping(Expanded);
   expandPresetsSpaceBeforeParens(Expanded);
+  Expanded.InsertBraces = false;
+  Expanded.RemoveBracesLLVM = false;
+  Expanded.RemoveSemicolon = false;
   switch (Expanded.RequiresClausePosition) {
   case FormatStyle::RCPS_SingleLine:
   case FormatStyle::RCPS_WithPreceding:
@@ -3403,20 +3402,26 @@
 }
 
 if (Style.InsertBraces) {
+  FormatStyle S = Expanded;
+  S.InsertBraces = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveBracesLLVM) {
+  FormatStyle S = Expanded;
+  S.RemoveBracesLLVM = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveSemicolon) {
+  FormatStyle S = Expanded;
+  S.RemoveSemicolon = true;
   Passes.emplace_back([&](const Environment ) {
-return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return SemiRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 


Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1907,9 +1907,7 @@
 class BracesInserter : public TokenAnalyzer {
 public:
   BracesInserter(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.RemoveBracesLLVM = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -1962,9 +1960,7 @@
 class BracesRemover : public TokenAnalyzer {
 public:
   BracesRemover(const Environment , const FormatStyle )
-  : TokenAnalyzer(Env, Style) {
-this->Style.InsertBraces = false;
-  }
+  : TokenAnalyzer(Env, Style) {}
 
   std::pair
   analyze(TokenAnnotator ,
@@ -3347,6 +3343,9 @@
   FormatStyle Expanded = Style;
   expandPresetsBraceWrapping(Expanded);
   expandPresetsSpaceBeforeParens(Expanded);
+  Expanded.InsertBraces = false;
+  Expanded.RemoveBracesLLVM = false;
+  Expanded.RemoveSemicolon = false;
   switch (Expanded.RequiresClausePosition) {
   case FormatStyle::RCPS_SingleLine:
   case FormatStyle::RCPS_WithPreceding:
@@ -3403,20 +3402,26 @@
 }
 
 if (Style.InsertBraces) {
+  FormatStyle S = Expanded;
+  S.InsertBraces = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveBracesLLVM) {
+  FormatStyle S = Expanded;
+  S.RemoveBracesLLVM = true;
   Passes.emplace_back([&](const Environment ) {
-return BracesRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
 if (Style.RemoveSemicolon) {
+  FormatStyle S = Expanded;
+  S.RemoveSemicolon = true;
   Passes.emplace_back([&](const Environment ) {
-return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+return SemiRemover(Env, S).process(/*SkipAnnotation=*/true);
   });
 }
 
___
cfe-commits mailing list