[Lldb-commits] [PATCH] D61994: [CommandInterpreter] Refactor SourceInitFile

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this is a bit more readable. I've included some suggestions which I think could make it even better.

Since you're already rewriting this code, this might be a good time to raise a point I wanted to bring up some day. Should we be using `FileSpec` for code like this? The code already uses a combination of llvm and lldb path utilities, and I would argue that we should use llvm all the way for code like this (except that we go through the FileSystem abstraction for the reproducer stuff). I have two reasons for that:

- FileSpec is designed for efficient matching of abstract file paths which may not even exist on the host system. As such, this code will result in a bunch of strings being added to the global string pool for no reason. And in this case, you're definitely working files which do exist (or may exist) on the host. In fact, FileSpec now contains code which performs path simplifications which are not completely sound given a concrete filesystem -- it will simplify a path like `bar/../foo` to `foo`, which is not correct if `bar` is a symlink.
- Since we started supporting windows paths, the FileSpec class offers more freedom than it is needed here. Specifically, it gives you the ability to return a foreign-syntax path as the "lldbinit" location. Therefore, in the spirit of using the most specific type which is sufficient to accomplish a given task, I think we should use a plain string here.



================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2149-2150
+
+  LoadCWDlldbinitFile should_load = ShouldLoadCwdInitFile();
+  if (should_load == eLoadCWDlldbinitFalse) {
+    result.SetStatus(eReturnStatusSuccessFinishNoResult);
----------------
Make this a `switch` ?


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2163
+    result.AppendErrorWithFormat(
+        "There is a .lldbinit file in the current directory which is not "
+        "being read.\n"
----------------
JDevlieghere wrote:
> This should be reflowed. 
What might help readability is moving the long block of text to a separate static variable declared before the function. That way the text does not obscure the logic of the function.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2191-2196
+    const char *program_name = program_file_spec.GetFilename().AsCString();
+
+    if (program_name) {
+      char program_init_file_name[PATH_MAX];
+      ::snprintf(program_init_file_name, sizeof(program_init_file_name),
+                  "%s-%s", init_file.GetPath().c_str(), program_name);
----------------
This should be done with strings and stringrefs


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61994