[NFC] Refactor and improve comments in CommandObjectTarget

[amccarth]

Summary

Made small improvements while debugging through CommandObjectTarget::AddModuleSymbols.

1. Refactored error case for an early out, reducing the indentation of the rest of this long function.
2. Clarified some comments by correcting spelling and punctuation.
3. Reduced duplicate code at the end of the function.

Tested with ninja check-lldb

[labath]

lldb/source/Commands/CommandObjectTarget.cpp

4059

This does change behavior because previously the symbol file directory wasn't being copied. I think that was intentional because the comment on line 4112 says "match up the file by basename" (and it also makes sense because if you're adding symbols in an external file, then the main module file cannot be the exact same path as the symbol file).

[amccarth]

lldb/source/Commands/CommandObjectTarget.cpp

4059

Oops. Thanks for catching that.

ModuleSpec seems weird: It exposes an internal members to be tweaked in arbitrary ways. I would have expected that it would have to react to certain kinds of changes to keep itself consistent. If it has no invariants to enforce, it could have been a plain struct with a bunch of public member variables.

[amccarth]

Reverted unintended change caught by reviewer.

[labath]

lldb/source/Commands/CommandObjectTarget.cpp

4059

This isn't really a matter of internal consistency. Both things make sense -- sometimes you want to match an actual full path, and sometimes only the basename.

That said, module spec does not have any internal consistency checks, and I don't think any are needed, so I think that replacing all the getters with public members would be a good idea.

[amccarth]

This landed in 3b69f0c5550a229dd6d39e361182cdd7cecc36a4, but there was a typo in the patch description so the tools didn't automatically close this.

lldb/source/Commands/CommandObjectTarget.cpp

public:
   }
 
   Options *GetOptions() override { return &m_option_group; }
 
 protected:
   bool AddModuleSymbols(Target *target, ModuleSpec &module_spec, bool &flush,
                         CommandReturnObject &result) {
     const FileSpec &symbol_fspec = module_spec.GetSymbolFileSpec();
-    if (symbol_fspec) {
+    if (!symbol_fspec) {
+      result.AppendError(
+          "one or more executable image paths must be specified");
+      result.SetStatus(eReturnStatusFailed);
+      return false;
+    }
+
     char symfile_path[PATH_MAX];
     symbol_fspec.GetPath(symfile_path, sizeof(symfile_path));
 
     if (!module_spec.GetUUID().IsValid()) {
       if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
         module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
     }
+
     // We now have a module that represents a symbol file that can be used
     // for a module that might exist in the current target, so we need to
     // find that module in the target
     ModuleList matching_module_list;
 
     size_t num_matches = 0;
     // First extract all module specs from the symbol file
     lldb_private::ModuleSpecList symfile_module_specs;
bool AddModuleSymbols(Target *target, ModuleSpec &module_spec, bool &flush,
     if (num_matches == 0) {
       target->GetImages().FindModules(module_spec, matching_module_list);
       num_matches = matching_module_list.GetSize();
     }
 
     while (num_matches == 0) {
       ConstString filename_no_extension(
           module_spec.GetFileSpec().GetFileNameStrippingExtension());
-        // Empty string returned, lets bail
+      // Empty string returned, let's bail
       if (!filename_no_extension)
         break;
 
-        // Check if there was no extension to strip and the basename is the
-        // same
+      // Check if there was no extension to strip and the basename is the same
       if (filename_no_extension == module_spec.GetFileSpec().GetFilename())
         break;
 
-        // Replace basename with one less extension
+      // Replace basename with one fewer extension
       module_spec.GetFileSpec().GetFilename() = filename_no_extension;
-
       target->GetImages().FindModules(module_spec, matching_module_list);
       num_matches = matching_module_list.GetSize();
     }
 
     if (num_matches > 1) {
       result.AppendErrorWithFormat("multiple modules match symbol file '%s', "
                                    "use the --uuid option to resolve the "
                                    "ambiguity.\n",
if (num_matches > 1) {
           return true;
         }
       }
       // Clear the symbol file spec if anything went wrong
       module_sp->SetSymbolFileFileSpec(FileSpec());
     }
 
     namespace fs = llvm::sys::fs;
-      if (module_spec.GetUUID().IsValid()) {
     StreamString ss_symfile_uuid;
+    if (module_spec.GetUUID().IsValid()) {
+      ss_symfile_uuid << " (";
       module_spec.GetUUID().Dump(&ss_symfile_uuid);
+      ss_symfile_uuid << ')';
+    }
     result.AppendErrorWithFormat(
-            "symbol file '%s' (%s) does not match any existing module%s\n",
+        "symbol file '%s'%s does not match any existing module%s\n",
         symfile_path, ss_symfile_uuid.GetData(),
         !fs::is_regular_file(symbol_fspec.GetPath())
             ? "\n       please specify the full path to the symbol file"
             : "");
-      } else {
-        result.AppendErrorWithFormat(
-            "symbol file '%s' does not match any existing module%s\n",
-            symfile_path,
-            !fs::is_regular_file(symbol_fspec.GetPath())
-                ? "\n       please specify the full path to the symbol file"
-                : "");
-      }
-    } else {
-      result.AppendError(
-          "one or more executable image paths must be specified");
-    }
     result.SetStatus(eReturnStatusFailed);
     return false;
   }
 
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     Target *target = m_exe_ctx.GetTargetPtr();
     result.SetStatus(eReturnStatusFailed);
     bool flush = false;