[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–770

What's the justification for this one?

[apelete]

lib/Frontend/CompilerInstance.cpp

763–770

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–770

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

LGTM

[klimek]

lib/Format/AffectedRangeManager.cpp

102

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

[djasper]

lib/Format/AffectedRangeManager.cpp

102

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/AST/DeclObjC.cpp

if (!ImplDecl->ivar_empty()) {
       if (!layout.empty()) {
         // Order synthesized ivars by their size.
         std::stable_sort(layout.begin(), layout.end());
         unsigned Ix = 0, EIx = layout.size();
         if (!data().IvarList) {
           data().IvarList = layout[0].Ivar; Ix++;
           curIvar = data().IvarList;
         }
-        for ( ; Ix != EIx; curIvar = layout[Ix].Ivar, Ix++)
+        for ( ; Ix != EIx; curIvar = layout[Ix].Ivar, Ix++) {
+          assert(curIvar && "instance variable is NULL, stop iterating through layout");
           curIvar->setNextIvar(layout[Ix].Ivar);
         }
       }
     }
+  }
   return data().IvarList;
 }
 
 /// FindCategoryDeclaration - Finds category declaration in the list of
 /// categories for this class and returns it. Name of the category is passed
 /// in 'CategoryId'. If category not found, return 0;
 ///
 ObjCCategoryDecl *

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(
     SmallVectorImpl<AnnotatedLine *>::iterator E) {
   while (I != E) {
     (*I)->Affected = true;
     markAllAsAffected((*I)->Children.begin(), (*I)->Children.end());
     ++I;
   }
 }
 
-bool AffectedRangeManager::nonPPLineAffected(
-    AnnotatedLine *Line, const AnnotatedLine *PreviousLine) {
+bool AffectedRangeMLanager::nonPPLineAffected(
+    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

lib/Frontend/CompilerInstance.cpp

assert(SourceMgr.getMainFileID().isValid() &&
            "Couldn't establish MainFileID!");
     return true;
   }
 
   StringRef InputFile = Input.getFile();
 
   // Figure out where to get and map in the main file.
   if (InputFile != "-") {
-    const FileEntry *File;
+    const FileEntry *File = nullptr;
     if (Opts.FindPchSource.empty()) {
       File = FileMgr.getFile(InputFile, /*OpenFile=*/true);
     } else {
       // When building a pch file in clang-cl mode, the .h file is built as if
       // it was included by a cc file.  Since the driver doesn't know about
       // all include search directories, the frontend must search the input
       // file through HeaderSearch here, as if it had been included by the
       // cc file at Opts.FindPchSource.
       const FileEntry *FindFile = FileMgr.getFile(Opts.FindPchSource);
       if (!FindFile) {
         Diags.Report(diag::err_fe_error_reading) << Opts.FindPchSource;
         return false;
       }
       const DirectoryLookup *UnusedCurDir;
       SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 16>
           Includers;
       Includers.push_back(std::make_pair(FindFile, FindFile->getDir()));
+      if (HS)
         File = HS->LookupFile(InputFile, SourceLocation(), /*isAngled=*/false,
                               /*FromDir=*/nullptr,
                               /*CurDir=*/UnusedCurDir, Includers,
                               /*SearchPath=*/nullptr,
                               /*RelativePath=*/nullptr,
                               /*RequestingModule=*/nullptr,
                               /*SuggestedModule=*/nullptr, /*SkipCache=*/true);
       // Also add the header to /showIncludes output.
       if (File)
         DepOpts.ShowIncludesPretendHeader = File->getName();
     }
     if (!File) {
       Diags.Report(diag::err_fe_error_reading) << InputFile;
       return false;
     }

lib/Sema/SemaExprCXX.cpp

return new (Context) CXXTypeidExpr(TypeInfoType.withConst(), Operand,
                                      SourceRange(TypeidLoc, RParenLoc));
 }
 
 /// \brief Build a C++ typeid expression with an expression operand.
 ExprResult Sema::BuildCXXTypeId(QualType TypeInfoType,
                                 SourceLocation TypeidLoc,
                                 Expr *E,
                                 SourceLocation RParenLoc) {
+  assert(E && "expression operand is NULL, cannot build C++ typeid expression");
+
   bool WasEvaluated = false;
-  if (E && !E->isTypeDependent()) {
+  if (!E->isTypeDependent()) {
     if (E->getType()->isPlaceholderType()) {
       ExprResult result = CheckPlaceholderExpr(E);
       if (result.isInvalid()) return ExprError();
       E = result.get();
     }
 
     QualType T = E->getType();
     if (const RecordType *RecordT = T->getAs<RecordType>()) {

lib/Sema/SemaLookup.cpp

static void checkCorrectionVisibility(Sema &SemaRef, TypoCorrection &TC) {
   // Nothing to do if all decls are visible.
   if (DI == DE)
     return;
 
   llvm::SmallVector<NamedDecl*, 4> NewDecls(TC.begin(), DI);
   bool AnyVisibleDecls = !NewDecls.empty();
 
   for (/**/; DI != DE; ++DI) {
+    assert(*DI && "TypoCorrection iterator cannot be NULL");
     NamedDecl *VisibleDecl = *DI;
     if (!LookupResult::isVisible(SemaRef, *DI))
       VisibleDecl = findAcceptableDecl(SemaRef, *DI);
 
     if (VisibleDecl) {
       if (!AnyVisibleDecls) {
         // Found a visible decl, discard all hidden ones.
         AnyVisibleDecls = true;

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

void PlistDiagnostics::FlushDiagnosticsImpl(
   // ranges of the diagnostics.
   FIDMap FM;
   SmallVector<FileID, 10> Fids;
   const SourceManager* SM = nullptr;
 
   if (!Diags.empty())
     SM = &Diags.front()->path.front()->getLocation().getManager();
 
+  assert(SM && "SourceManager is NULL, cannot iterate through the diagnostics");
 
   for (std::vector<const PathDiagnostic*>::iterator DI = Diags.begin(),
        DE = Diags.end(); DI != DE; ++DI) {
 
     const PathDiagnostic *D = *DI;
 
     SmallVector<const PathPieces *, 5> WorkList;
     WorkList.push_back(&D->path);