[Lldb-commits] [PATCH] D77186: [source maps] Ensure all valid source maps are added instead of failing with the first invalid one

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It seems somewhat odd for a command to return error (one of the effects of that for instance is to abort processing of batch scripts), but still perform some changes. It might be more appropriate to call those warnings. However, reporting warnings from here would require some plumbing, and the new behavior is definitely more reasonable than the old one, so I don't think this patch should be blocked on that.



================
Comment at: lldb/source/Interpreter/OptionValuePathMappings.cpp:73
             changed = true;
+            idx++;
           } else {
----------------
does this actually change anything?


================
Comment at: lldb/source/Interpreter/OptionValuePathMappings.cpp:113-114
         } else {
           error.SetErrorStringWithFormat(
               "the replacement path doesn't exist: \"%s\"", replace_path);
         }
----------------
If there are multiple non-existing paths, this will just return the last one right? We should list all of them.


================
Comment at: lldb/source/Interpreter/OptionValuePathMappings.cpp:146
             changed = true;
+            idx++;
           } else {
----------------
does this change anything?


================
Comment at: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py:45
+        src_map_cmd = 'settings set target.source-map . "%s" . "%s"' % (src_dir + "invalid_path", src_dir)
         self.dbg.HandleCommand(src_map_cmd)
+        assertSourceMaps(src_path)
----------------
It would be good to follow this up with a `settings show`, both to test/document the expected final value of the setting, and also to catch any issues early.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77186/new/

https://reviews.llvm.org/D77186