This is an archive of the discontinued LLVM Phabricator instance.

[CommandInterpreter] Refactor SourceInitFile
ClosedPublic

Authored by JDevlieghere on May 15 2019, 11:28 PM.

Details

Summary

I was looking at the current implementation of SourceInitFile and there were a few things that made this function hard to read. The code to find the ~/.lldbinit file is duplicated across the cwd and non-cwd branch. The ./.lldbinit is once computed by resolving .lldbinit and once by resolving ./.lldbinit. Furthermore it wasn't clear to me what happened when you're sourcing the .lldbinit file in the current working directory. Apparently we do nothing when we property to control that is set to warn (makes sense) and we don't care when the property is set to true (debatable). Finally, there were at least two branches where the status of the CommandReturnObject were not set.

Anyway, this is an attempt to simplify this code a bit. It's still more complex than I had hoped.

Please let me know if you think it's more readable or not.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.May 15 2019, 11:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 11:28 PM
JDevlieghere marked 2 inline comments as done.May 15 2019, 11:29 PM
JDevlieghere added inline comments.
lldb/source/Interpreter/CommandInterpreter.cpp
2118 ↗(On Diff #199753)

This should also have an assert(!m_skip_lldbinit_files);

2163 ↗(On Diff #199753)

This should be reflowed.

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.
lldb/source/Interpreter/CommandInterpreter.cpp
2149–2150 ↗(On Diff #199753)

Make this a switch ?

2163 ↗(On Diff #199753)

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.

2191–2196 ↗(On Diff #199753)

This should be done with strings and stringrefs

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

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.

lldb/source/Interpreter/CommandInterpreter.cpp
2149–2150 ↗(On Diff #199753)

+1

2163 ↗(On Diff #199753)

I agree with Pavel, that would indeed make things more readable.

Address code review feedback

JDevlieghere marked 5 inline comments as done.May 16 2019, 5:22 PM
labath accepted this revision.May 17 2019, 12:53 AM

I think this looks better than the original. Given that the only callers of SourceInitFile pass a constant value for the InitFileLocation argument, what I'd consider also doing is getting rid of the function and the enum, and making SourceInitFileHome/SourceInitFileCwd the main entry points.

One day I'm going to start a thread about reducing the use of FileSpecs in the FileSystem api...

This revision is now accepted and ready to land.May 17 2019, 12:53 AM

Remove unneeded use of FileSpec. I wanted to do this yesterday but didn't get around to it...

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 3:51 PM