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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
