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