[PATCH] D149647: [NFC][Clang]Fix static analyzer tool remarks about large copies by values

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/Format.cpp:3486-3489
+      Expanded.InsertBraces = true;
+      Passes.emplace_back([&](const Environment &Env) {
+        return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
       });
----------------
tahonermann wrote:
> HazardyKnusperkeks wrote:
> > HazardyKnusperkeks wrote:
> > > HazardyKnusperkeks wrote:
> > > > MyDeveloperDay wrote:
> > > > > owenpan wrote:
> > > > > > tahonermann wrote:
> > > > > > > How about using an init capture instead? This will suffice to avoid one of the copies but means that `InsertBraces` doesn't get set until the lambda is invoked. I wouldn't expect that to matter though.
> > > > > > I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.
> > > > > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this bascially do the same without us having to remember to put Expanded back afterwards? I don't see how using Expanded is any different from using S (other than the copy)
> > > > > 
> > > > > ```
> > > > > if (Style.InsertBraces) {
> > > > >       FormatStyle S = Expanded;
> > > > >       S.InsertBraces = true;
> > > > >       Passes.emplace_back([&](const Environment &Env) {
> > > > >         return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > > > >       });
> > > > >     }
> > > > > ```
> > > > > I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.
> > > > 
> > > > That wouldn't work, since the pass is only executed afterwards.
> > > > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this bascially do the same without us having to remember to put Expanded back afterwards? I don't see how using Expanded is any different from using S (other than the copy)
> > > > 
> > > > ```
> > > > if (Style.InsertBraces) {
> > > >       FormatStyle S = Expanded;
> > > >       S.InsertBraces = true;
> > > >       Passes.emplace_back([&](const Environment &Env) {
> > > >         return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > > >       });
> > > >     }
> > > > ```
> > > 
> > > That would have a dangling reference to S and most likely don't work as wished.
> > This should work. One could replace the two assignments with a RAII wrapper which restores the old value.
> Here is another option; it is just the original code but with an init capture that does a move instead of a copy. Coverity shouldn't complain about a move capture (if it does, I'll file a support case with Synopsys).
> > I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.
> 
> That wouldn't work, since the pass is only executed afterwards.

You're right!


================
Comment at: clang/lib/Format/Format.cpp:3486-3489
+      Expanded.InsertBraces = true;
+      Passes.emplace_back([&](const Environment &Env) {
+        return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
       });
----------------
owenpan wrote:
> tahonermann wrote:
> > HazardyKnusperkeks wrote:
> > > HazardyKnusperkeks wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > MyDeveloperDay wrote:
> > > > > > owenpan wrote:
> > > > > > > tahonermann wrote:
> > > > > > > > How about using an init capture instead? This will suffice to avoid one of the copies but means that `InsertBraces` doesn't get set until the lambda is invoked. I wouldn't expect that to matter though.
> > > > > > > I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.
> > > > > > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this bascially do the same without us having to remember to put Expanded back afterwards? I don't see how using Expanded is any different from using S (other than the copy)
> > > > > > 
> > > > > > ```
> > > > > > if (Style.InsertBraces) {
> > > > > >       FormatStyle S = Expanded;
> > > > > >       S.InsertBraces = true;
> > > > > >       Passes.emplace_back([&](const Environment &Env) {
> > > > > >         return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > > > > >       });
> > > > > >     }
> > > > > > ```
> > > > > > I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.
> > > > > 
> > > > > That wouldn't work, since the pass is only executed afterwards.
> > > > > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this bascially do the same without us having to remember to put Expanded back afterwards? I don't see how using Expanded is any different from using S (other than the copy)
> > > > > 
> > > > > ```
> > > > > if (Style.InsertBraces) {
> > > > >       FormatStyle S = Expanded;
> > > > >       S.InsertBraces = true;
> > > > >       Passes.emplace_back([&](const Environment &Env) {
> > > > >         return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > > > >       });
> > > > >     }
> > > > > ```
> > > > 
> > > > That would have a dangling reference to S and most likely don't work as wished.
> > > This should work. One could replace the two assignments with a RAII wrapper which restores the old value.
> > Here is another option; it is just the original code but with an init capture that does a move instead of a copy. Coverity shouldn't complain about a move capture (if it does, I'll file a support case with Synopsys).
> > > I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.
> > 
> > That wouldn't work, since the pass is only executed afterwards.
> 
> You're right!
> > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this bascially do the same without us having to remember to put Expanded back afterwards? I don't see how using Expanded is any different from using S (other than the copy)
> > 
> > ```
> > if (Style.InsertBraces) {
> >       FormatStyle S = Expanded;
> >       S.InsertBraces = true;
> >       Passes.emplace_back([&](const Environment &Env) {
> >         return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> >       });
> >     }
> > ```
> 
> That would have a dangling reference to S and most likely don't work as wished.

+1. See D140058#4000311.


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

https://reviews.llvm.org/D149647