refactor/cleanup ClangExpressionParser::Parse

[ldrumm]

Summary

• fix return type: ClangExpressionParser::Parse returns unsigned, but was actually returning a signed value, num_errors.
• use helper clang::TextDiagnosticBuffer::getNumErrors() instead of counting the errors ourself.
• limit scoping of block-level automatic variables as much as practical.
• remove reused multipurpose TextDiagnosticBuffer::const_iterator in favour of loop-scoped err, warn, and note variables in the diagnostic printing code.
• refactor diagnostic printing loops to use a proper loop invariant.

[clayborg]

I will let Sean Callanan OK this one.

[spyffe]

Aside from a minor nit, this looks like good cleanup. If it passes the testsuite, feel free to commit after addressing the inline comment.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

522 ↗

(On Diff #48002)

I'm okay with use of auto when the type is really onerous to express in code, but in this case TextDiagnosticBuffer::const_iterator is pretty compact, and makes the type of warn more obvious. Could we use that instead of auto?

lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

m_code_generator.reset(CreateLLVMCodeGen(m_compiler->getDiagnostics(),
                                              *m_llvm_context));
 }
 
 ClangExpressionParser::~ClangExpressionParser()
 {
 }
 
 unsigned
-ClangExpressionParser::Parse (Stream &stream)
+ClangExpressionParser::Parse(Stream &stream)
 {
-    TextDiagnosticBuffer *diag_buf = static_cast<TextDiagnosticBuffer*>(m_compiler->getDiagnostics().getClient());
-
-    diag_buf->FlushDiagnostics (m_compiler->getDiagnostics());
+    TextDiagnosticBuffer *diag_buf = static_cast<TextDiagnosticBuffer *>(m_compiler->getDiagnostics().getClient());
+    diag_buf->FlushDiagnostics(m_compiler->getDiagnostics());
 
     const char *expr_text = m_expr.Text();
 
-    clang::SourceManager &SourceMgr = m_compiler->getSourceManager();
+    clang::SourceManager &source_mgr = m_compiler->getSourceManager();
     bool created_main_file = false;
     if (m_compiler->getCodeGenOpts().getDebugInfo() == codegenoptions::FullDebugInfo)
     {
-        std::string temp_source_path;
-
         int temp_fd = -1;
         llvm::SmallString<PATH_MAX> result_path;
         FileSpec tmpdir_file_spec;
         if (HostInfo::GetLLDBPath(lldb::ePathTypeLLDBTempSystemDir, tmpdir_file_spec))
         {
             tmpdir_file_spec.AppendPathComponent("lldb-%%%%%%.expr");
-            temp_source_path = tmpdir_file_spec.GetPath();
+            std::string temp_source_path = tmpdir_file_spec.GetPath();
             llvm::sys::fs::createUniqueFile(temp_source_path, temp_fd, result_path);
         }
         else
         {
             llvm::sys::fs::createTemporaryFile("lldb", "expr", temp_fd, result_path);
         }
-        
         if (temp_fd != -1)
         {
-            lldb_private::File file (temp_fd, true);
+            lldb_private::File file(temp_fd, true);
             const size_t expr_text_len = strlen(expr_text);
             size_t bytes_written = expr_text_len;
             if (file.Write(expr_text, bytes_written).Success())
             {
                 if (bytes_written == expr_text_len)
                 {
                     file.Close();
-                    SourceMgr.setMainFileID(SourceMgr.createFileID(
-                        m_file_manager->getFile(result_path),
+                    source_mgr.setMainFileID(source_mgr.createFileID(m_file_manager->getFile(result_path),
                                                                      SourceLocation(), SrcMgr::C_User));
                     created_main_file = true;
                 }
             }
         }
     }
 
     if (!created_main_file)
     {
         std::unique_ptr<MemoryBuffer> memory_buffer = MemoryBuffer::getMemBufferCopy(expr_text, __FUNCTION__);
-        SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(memory_buffer)));
+        source_mgr.setMainFileID(source_mgr.createFileID(std::move(memory_buffer)));
     }
 
     diag_buf->BeginSourceFile(m_compiler->getLangOpts(), &m_compiler->getPreprocessor());
 
     ClangExpressionHelper *type_system_helper = dyn_cast<ClangExpressionHelper>(m_expr.GetTypeSystemHelper());
 
     ASTConsumer *ast_transformer = type_system_helper->ASTTransformer(m_code_generator.get());
 
     if (ClangExpressionDeclMap *decl_map = type_system_helper->DeclMap())
         decl_map->InstallCodeGenerator(m_code_generator.get());
 
     if (ast_transformer)
     {
         ast_transformer->Initialize(m_compiler->getASTContext());
         ParseAST(m_compiler->getPreprocessor(), ast_transformer, m_compiler->getASTContext());
     }
     else
     {
         m_code_generator->Initialize(m_compiler->getASTContext());
         ParseAST(m_compiler->getPreprocessor(), m_code_generator.get(), m_compiler->getASTContext());
     }
 
     diag_buf->EndSourceFile();
 
-    TextDiagnosticBuffer::const_iterator diag_iterator;
-
-    int num_errors = 0;
-    
+    unsigned num_errors = diag_buf->getNumErrors();
     if (m_pp_callbacks && m_pp_callbacks->hasErrors())
     {
         num_errors++;
-        
         stream.PutCString(m_pp_callbacks->getErrorString().c_str());
     }
 
-    for (diag_iterator = diag_buf->warn_begin();
-         diag_iterator != diag_buf->warn_end();
-         ++diag_iterator)
-        stream.Printf("warning: %s\n", (*diag_iterator).second.c_str());
-
-    for (diag_iterator = diag_buf->err_begin();
-         diag_iterator != diag_buf->err_end();
-         ++diag_iterator)
-    {
-        num_errors++;
-        stream.Printf("error: %s\n", (*diag_iterator).second.c_str());
-    }
-
-    for (diag_iterator = diag_buf->note_begin();
-         diag_iterator != diag_buf->note_end();
-         ++diag_iterator)
-        stream.Printf("note: %s\n", (*diag_iterator).second.c_str());
+    for (TextDiagnosticBuffer::const_iterator warn = diag_buf->warn_begin(), warn_end = diag_buf->warn_end();
+         warn != warn_end; ++warn)
+        stream.Printf("warning: %s\n", warn->second.c_str());
+
+    for (TextDiagnosticBuffer::const_iterator err = diag_buf->err_begin(), err_end = diag_buf->err_end();
+         err != err_end; ++err)
+        stream.Printf("error: %s\n", err->second.c_str());
+
+    for (TextDiagnosticBuffer::const_iterator note = diag_buf->note_begin(), note_end = diag_buf->note_end();
+         note != note_end; ++note)
+        stream.Printf("note: %s\n", note->second.c_str());
 
     if (!num_errors)
     {
         if (type_system_helper->DeclMap() && !type_system_helper->DeclMap()->ResolveUnknownTypes())
         {
             stream.Printf("error: Couldn't infer the type of a variable\n");
             num_errors++;
         }