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

[Manna]

Summary

Reported by Static Analyzer Tool:

Inside "Format.cpp" file, in

clang::format::internal::reformat(clang::format::FormatStyle const &, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus *)::[lambda(clang::format::Environment const &) (instance 4)]::operator ()(clang::format::Environment const &): A very large function call parameter exceeding the high threshold is passed by value.

pass_by_value: Capturing variable S of type clang::format::FormatStyle (size 808 bytes) by value, which exceeds the high threshold of 512 bytes

This patch removes redundant capturing variable S and passes type clang::format::FormatStyle Expanded instead as parameter in functions: BracesInserter(), BracesRemover(), and SemiRemover().

[tahonermann]

Looks good to me.

[Manna]

Thank you @erichkeane and @tahonermann for reviews!

[HazardyKnusperkeks]

But it is plain wrong.
It was done on purpose, so that e.g. RemoveBracesLLVM is not set when the SemiRemover does its work.

[erichkeane]

In D149647#4313311, @HazardyKnusperkeks wrote:

But it is plain wrong.
It was done on purpose, so that e.g. RemoveBracesLLVM is not set when the SemiRemover does its work.

Ah, shoot, you're right! I missed that the 'return' was in a lambda! Thanks for the good catch! I think this patch is just 'wrong' in its entirety, and this is a case where Coverity needs to be suppressed.

[tahonermann]

Thank you for catching that @HazardyKnusperkeks! I completely missed (somehow) that the changed code modified Expanded. I offered another suggestion.

clang/lib/Format/Format.cpp

3486–3489

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.

[owenpan]

clang/lib/Format/Format.cpp

3486–3489

I'm not sure if it's worth the trouble, but if I really had to bother, I would do something like the above.

[MyDeveloperDay]

Can you link to the Coverity issue so we can see what it was complaining about?

[erichkeane]

In D149647#4316134, @MyDeveloperDay wrote:

Can you link to the Coverity issue so we can see what it was complaining about?

Unfortunately this is from an internal-run of Coverity that Intel is running. I'd be shocked if it doesn't appear on the open-source/LLVM version though?

[Manna]

Thanks @erichkeane for response.

@MyDeveloperDay, Our internal Coverity link won't work without login.
This is what Coverity was complaining about.

Big parameter passed by value
Copying large values is inefficient, consider passing by reference; Low, medium, and high size thresholds for detection can be adjusted.

1. In clang::​format::​internal::​reformat(clang::​format::​FormatStyle const &, llvm::​StringRef, llvm::​ArrayRef<clang::​tooling::​Range>, unsigned int, unsigned int, unsigned int, llvm::​StringRef, clang::​format::​FormattingAttemptStatus *)::​[lambda(clang::​format::​Environment const &) (instance 5)]::​operator ()(clang::​format::​Environment const &): A very large function call parameter exceeding the high threshold is passed by value. (CWE-398)

if (Style.RemoveSemicolon) {
3499 FormatStyle S = Expanded;
3500 S.RemoveSemicolon = true;

pass_by_value: Capturing variable S of type clang::format::FormatStyle (size 572 bytes) by value, which exceeds the high threshold of 512 bytes.

3501 Passes.emplace_back([&, S](const Environment &Env) {
3502 return SemiRemover(Env, S).process(/*SkipAnnotation=*/true);
3503 });
3504 }

2. In clang::​format::​internal::​reformat(clang::​format::​FormatStyle const &, llvm::​StringRef, llvm::​ArrayRef<clang::​tooling::​Range>, unsigned int, unsigned int, unsigned int, llvm::​StringRef, clang::​format::​FormattingAttemptStatus *)::​[lambda(clang::​format::​Environment const &) (instance 4)]::​operator ()(clang::​format::​Environment const &): A very large function call parameter exceeding the high threshold is passed by value. (CWE-398)

if (Style.RemoveBracesLLVM) {
3491 FormatStyle S = Expanded;
3492 S.RemoveBracesLLVM = true;

pass_by_value: Capturing variable S of type clang::format::FormatStyle (size 572 bytes) by value, which exceeds the high threshold of 512 bytes.

3493 Passes.emplace_back([&, S](const Environment &Env) {
3494 return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
3495 });
3496 }

3. In clang::​format::​internal::​reformat(clang::​format::​FormatStyle const &, llvm::​StringRef, llvm::​ArrayRef<clang::​tooling::​Range>, unsigned int, unsigned int, unsigned int, llvm::​StringRef, clang::​format::​FormattingAttemptStatus *)::​[lambda(clang::​format::​Environment const &) (instance 3)]::​operator ()(clang::​format::​Environment const &): A very large function call parameter exceeding the high threshold is passed by value. (CWE-398)

if (Style.InsertBraces) {
3483 FormatStyle S = Expanded;
3484 S.InsertBraces = true;

pass_by_value: Capturing variable S of type clang::format::FormatStyle (size 572 bytes) by value, which exceeds the high threshold of 512 bytes.

3485 Passes.emplace_back([&, S](const Environment &Env) {
3486 return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
3487 });
3488 }

In D149647#4316169, @erichkeane wrote:

In D149647#4316134, @MyDeveloperDay wrote:

Can you link to the Coverity issue so we can see what it was complaining about?

Unfortunately this is from an internal-run of Coverity that Intel is running. I'd be shocked if it doesn't appear on the open-source/LLVM version though?

[MyDeveloperDay]

ok! I'm not a massive lambda expert, but aren't we passing by const reference e.g. const FormatStyle &Style, can someone give me a lession as to why it thinks its by value? maybe I'm looking at the wrong "pass by"

[MyDeveloperDay]

clang/lib/Format/Format.cpp

3486–3489

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);
      });
    }

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

3486–3489

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.

3486–3489

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.

3486–3489

This should work. One could replace the two assignments with a RAII wrapper which restores the old value.

[tahonermann]

clang/lib/Format/Format.cpp

3486–3489

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).

[Manna]

Thank you for the reviews and feedbacks. I have applied suggestion about init capture that does a move instead of a copy.

[owenpan]

clang/lib/Format/Format.cpp

3486–3489

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!

3486–3489

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.

[tahonermann]

Looks good. Thanks again for catching our oops @HazardyKnusperkeks!

[Manna]

Thank you everyone for reviews and comments.

[Manna]

Thanks again for catching our oops @HazardyKnusperkeks!

Yup, Thank you @HazardyKnusperkeks!

clang/lib/Format/Format.cpp

if (Style.QualifierAlignment != FormatStyle::QAS_Leave) {
         return QualifierAlignmentFixer(Env, Expanded, Code, Ranges,
                                        FirstStartColumn, NextStartColumn,
                                        LastStartColumn, FileName)
             .process();
       });
     }
 
     if (Style.InsertBraces) {
-      FormatStyle S = Expanded;
-      S.InsertBraces = true;
-      Passes.emplace_back([&, S](const Environment &Env) {
-        return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
+      Expanded.InsertBraces = true;
+      Passes.emplace_back([&](const Environment &Env) {
+        return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
       });
     }
 
     if (Style.RemoveBracesLLVM) {
-      FormatStyle S = Expanded;
-      S.RemoveBracesLLVM = true;
-      Passes.emplace_back([&, S](const Environment &Env) {
-        return BracesRemover(Env, S).process(/*SkipAnnotation=*/true);
+      Expanded.RemoveBracesLLVM = true;
+      Passes.emplace_back([&](const Environment &Env) {
+        return BracesRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
       });
     }
 
     if (Style.RemoveSemicolon) {
-      FormatStyle S = Expanded;
-      S.RemoveSemicolon = true;
-      Passes.emplace_back([&, S](const Environment &Env) {
-        return SemiRemover(Env, S).process(/*SkipAnnotation=*/true);
+      Expanded.RemoveSemicolon = true;
+      Passes.emplace_back([&](const Environment &Env) {
+        return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
       });
     }
 
     if (Style.FixNamespaceComments) {
       Passes.emplace_back([&](const Environment &Env) {
         return NamespaceEndCommentsFixer(Env, Expanded).process();
       });
     }