[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

103 ↗

(On Diff #55327)

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

[djasper]

lib/Format/AffectedRangeManager.cpp

103 ↗

(On Diff #55327)

Yeah, that makes sense to me.

111 ↗

(On Diff #55327)

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