[scan-build] fix warnings emitted on Clang Format code base

[apelete]

Summary

Fix "Logic error" warnings of the type "Called C++ object pointer is
null" reported by Clang Static Analyzer on the following file:

• lib/Format/AffectedRangeManager.cpp.

Signed-off-by: Apelete Seketeli <[e-mail]>

[rjmccall]

lib/Frontend/CompilerInstance.cpp

763 ↗

(On Diff #54564)

What's the justification for this one?

[apelete]

lib/Frontend/CompilerInstance.cpp

763 ↗

(On Diff #54564)

CompilerInstance::InitializeSourceManager() could call CompilerInstance::InitializeSourceManager() and pass a null pointer value via the 5th parameter 'HS':

718. bool CompilerInstance::InitializeSourceManager(const FrontendInputFile &Input){
719. return InitializeSourceManager(
720. Input, getDiagnostics(), getFileManager(), getSourceManager(),
721. hasPreprocessor() ? &getPreprocessor().getHeaderSearchInfo() : nullptr,
722. getDependencyOutputOpts(), getFrontendOpts());
723. }

In that case, 'HS' object pointer would be null at this point.
I chose not to assert 'HS' and just check it because we already check if 'File' is assigned a null pointer as the result of HS->LookupFile() below.

Should it be fixed otherwise ?

[rjmccall]

lib/Frontend/CompilerInstance.cpp

763 ↗

(On Diff #54564)

I don't really know.

The Sema and AST changes LGTM.

[apelete]

[scan-build] fix logic error warnings emitted on clang code base

Chnages since last revision:

• lib/Format/Format.cpp: removed all changes, redenderd obsolete by upstream.

[apelete]

[scan-build] fix logic error warnings emitted on clang code base

Changes since last revision:

• lib/Format/AffectedRangeManager.cpp: moved the fixes from lib/Format/Format.cpp:AffectedRangeManager::nonPPLineAffected() here since the code appaears to have been moved.

[zaks.anna]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

300–301 ↗

(On Diff #55327)

LGTM

[klimek]

lib/Format/AffectedRangeManager.cpp

103

Perhaps we should change this to take a Line& instead? Daniel, thoughts?

[djasper]

lib/Format/AffectedRangeManager.cpp

103

Yeah, that makes sense to me.

109

Here, we should probably just return false.

[apelete]

[scan-build] fix logic error warnings emitted on clang code base

Changes since last revison:

• lib/Format/AffectedRangeManager.h: fix declaration of nonPPLineAffected() to pass a reference to AnnotatedLine as first parameter.

• lib/Format/AffectedRangeManager.cpp: fix nonPPLineAffected() definition accordingly.

[apelete]

[scan-build] fix logic error warnings emitted on clang code base

Changes since last revision:

• lib/Format/AffectedRangeManager.cpp: fix the call to AffectedRangeManager::nonPPLineAffected() from AffectedRangeManager::computeAffectedLines() that was left out of previous revision somehow.

[apelete]

Thanks for your reviews.

[apelete]

[scan-build] fix logic error warnings emitted on clang code base

Changes since last revision:

• fast forward rebase on git master branch.

[apelete]

Waiting for review, could someone please have a look at this one ?

[apelete]

[scan-build] fix logic error warnings emitted on clang code base

Changes since last revision:

• lib/Format/AffectedRangeManager.cpp: fix typo in class name.

[apelete]

[scan-build] fix warnings emitted on Clang Format code base

Changes since last revision:

• split patch into Format unit to ease review process.

[djasper]

Looks good.

[shafik]

Is this relevant anymore or should it be closed?

[apelete]

I guess it can be closed, thanks.

lib/Format/AffectedRangeManager.h

private:
   bool affectsLeadingEmptyLines(const FormatToken &Tok);
 
   // Marks all lines between I and E as well as all their children as affected.
   void markAllAsAffected(SmallVectorImpl<AnnotatedLine *>::iterator I,
                          SmallVectorImpl<AnnotatedLine *>::iterator E);
 
   // Determines whether 'Line' is affected by the SourceRanges given as input.
   // Returns \c true if line or one if its children is affected.
-  bool nonPPLineAffected(AnnotatedLine *Line,
+  bool nonPPLineAffected(AnnotatedLine &Line,
                          const AnnotatedLine *PreviousLine);
 
   const SourceManager &SourceMgr;
   const SmallVector<CharSourceRange, 8> Ranges;
 };
 
 } // namespace format
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_FORMAT_AFFECTEDRANGEMANAGER_H

lib/Format/AffectedRangeManager.cpp

if (Line->InPPDirective) {
                             /*IncludeLeadingNewlines=*/false)) {
         SomeLineAffected = true;
         markAllAsAffected(I, PPEnd);
       }
       I = PPEnd;
       continue;
     }
 
-    if (nonPPLineAffected(Line, PreviousLine))
+    if (nonPPLineAffected(*Line, PreviousLine))
       SomeLineAffected = true;
 
     PreviousLine = Line;
     ++I;
   }
   return SomeLineAffected;
 }
 
void AffectedRangeManager::markAllAsAffected(
   while (I != E) {
     (*I)->Affected = true;
     markAllAsAffected((*I)->Children.begin(), (*I)->Children.end());
     ++I;
   }
 }
 
 bool AffectedRangeManager::nonPPLineAffected(
-    AnnotatedLine *Line, const AnnotatedLine *PreviousLine) {
+    AnnotatedLine &Line, const AnnotatedLine *PreviousLine) {
   bool SomeLineAffected = false;
-  Line->ChildrenAffected =
-      computeAffectedLines(Line->Children.begin(), Line->Children.end());
-  if (Line->ChildrenAffected)
+  Line.ChildrenAffected =
+      computeAffectedLines(Line.Children.begin(), Line.Children.end());
+  if (Line.ChildrenAffected)
     SomeLineAffected = true;
 
-  // Stores whether one of the line's tokens is directly affected.
-  bool SomeTokenAffected = false;
   // Stores whether we need to look at the leading newlines of the next token
   // in order to determine whether it was affected.
   bool IncludeLeadingNewlines = false;
 
   // Stores whether the first child line of any of this line's tokens is
   // affected.
   bool SomeFirstChildAffected = false;
 
-  for (FormatToken *Tok = Line->First; Tok; Tok = Tok->Next) {
+  // Stores whether one of the line's tokens is directly affected.
+  bool SomeTokenAffected = false;
+  if (Line.First == nullptr)
+    return SomeTokenAffected;
+
+  for (FormatToken *Tok = Line.First; Tok; Tok = Tok->Next) {
     // Determine whether 'Tok' was affected.
     if (affectsTokenRange(*Tok, *Tok, IncludeLeadingNewlines))
       SomeTokenAffected = true;
 
     // Determine whether the first child of 'Tok' was affected.
     if (!Tok->Children.empty() && Tok->Children.front()->Affected)
       SomeFirstChildAffected = true;
 
     IncludeLeadingNewlines = Tok->Children.empty();
   }
 
   // Was this line moved, i.e. has it previously been on the same line as an
   // affected line?
   bool LineMoved = PreviousLine && PreviousLine->Affected &&
-                   Line->First->NewlinesBefore == 0;
+                   Line.First->NewlinesBefore == 0;
 
   bool IsContinuedComment =
-      Line->First->is(tok::comment) && Line->First->Next == nullptr &&
-      Line->First->NewlinesBefore < 2 && PreviousLine &&
+      Line.First->is(tok::comment) && Line.First->Next == nullptr &&
+      Line.First->NewlinesBefore < 2 && PreviousLine &&
       PreviousLine->Affected && PreviousLine->Last->is(tok::comment);
 
   if (SomeTokenAffected || SomeFirstChildAffected || LineMoved ||
       IsContinuedComment) {
-    Line->Affected = true;
+    Line.Affected = true;
     SomeLineAffected = true;
   }
   return SomeLineAffected;
 }
 
 } // namespace format
 } // namespace clang