[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4059
       if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
-          module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
+        module_spec.GetFileSpec() = symbol_fspec;
     }
----------------
amccarth wrote:
> labath wrote:
> > 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).
> 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.
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.


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

https://reviews.llvm.org/D70458