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

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

Seems like an overall improvement to the structure of this code. At the high level, the structure feels easier to understand.

In D61994#1504336 <https://reviews.llvm.org/D61994#1504336>, @labath wrote:

> 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.


+1

> - 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.

I'm especially concerned about windows support, so I also agree with this.



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


================
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"
----------------
labath wrote:
> 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.
I agree with Pavel, that would indeed make things more readable.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61994