Page MenuHomePhabricator

Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands
ClosedPublic

Authored by JDevlieghere on Mar 8 2019, 2:15 PM.

Details

Summary

Inspired by Zachary's mail on lldb-dev, this seemed like low hanging fruit. This patch breaks the circular dependency between commands and expression.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Mar 8 2019, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2019, 2:15 PM
zturner added inline comments.Mar 8 2019, 2:28 PM
lldb/include/lldb/Commands/CommandObjectBreakpoint.h
36 ↗(On Diff #189935)

I think the clang-format got messed up. It appears to be formatting your entire tree instead of only the files that were changed in this patch. Can you try to re-run clang-format only on the changed files?

JDevlieghere marked 2 inline comments as done.Mar 8 2019, 2:33 PM
JDevlieghere added inline comments.
lldb/include/lldb/Commands/CommandObjectBreakpoint.h
36 ↗(On Diff #189935)

I did. I use git clang-format, which probably considers the whole file to be modified when you move it. It rings a bell but I don't remember the specifics.

The other files look fine though, so I can just reset this one.

I think maybe part of the problem is that this patch looks like actually 2 things. 1) A move of the include files from lldb/source/Commands to lldb/Include/lldb/Commands, and 2) The dependency changes. So it makes it hard to see what changes are actually needed for breaking the dependency.

Would it be possible to first move the header files as an independent change (which probably doesn't even need to be clang-formatted), and then after that fix the dependency issues?

lldb/source/Expression/REPL.cpp
10 ↗(On Diff #189935)

AFAICT, this doesn't really appear to break the dependency does it? Because right here, Expression will still cause a link dependency against Commands.

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.
lldb/source/Expression/REPL.cpp
10 ↗(On Diff #189935)

Yeah you're totally right, I'm doing too many things at the same time.

Interesting, I had looked at fixing this one once before but I didn't realize we had a class named EvaluateExpressionOptions that contained just the right set of information, and it felt gross to invent a new one just for this purpose (It's a good thing I didn't too, since we already had another one).

Can you also update the CMake files? You'll need to update Expression/CMakeLists.txt to not pull in lldbTarget. I was going to say that you would also need to update unittests/Expression/CMakeLists.txt, but it's already not listed, so I guess it's just been relying on linking it transitively.

Do you have the ability to test the build with CMake? If so, could you try ninja ExpressionTests and make sure everything still works after removing the link dependcy in Expression/CMakeLists.txt?

Interesting, I had looked at fixing this one once before but I didn't realize we had a class named EvaluateExpressionOptions that contained just the right set of information, and it felt gross to invent a new one just for this purpose (It's a good thing I didn't too, since we already had another one).

Can you also update the CMake files? You'll need to update Expression/CMakeLists.txt to not pull in lldbTarget. I was going to say that you would also need to update unittests/Expression/CMakeLists.txt, but it's already not listed, so I guess it's just been relying on linking it transitively.

You mean lldbCommand, right? It's already not listed there, and neither in the unittest.

Do you have the ability to test the build with CMake? If so, could you try ninja ExpressionTests and make sure everything still works after removing the link dependcy in Expression/CMakeLists.txt?

zturner accepted this revision.Mar 8 2019, 3:56 PM

Ahh yea, sorry. Got confused for a second :)

This revision is now accepted and ready to land.Mar 8 2019, 3:56 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2019, 4:09 PM

It's a little weird that the CommandObjectExpression is telling the REPL what its options should be, it would be more natural to go the other way. Maybe have the repl_sp provide a properly set up set of EvaluateExpressionOptions and then copy them over to this CommandObjectExpression execution?

Also, as is GetExprOptions is not a very descriptive name for something that's specifically setting up Command & EvaluateExpressionOptions for the REPL.