[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src
Raphael Isemann via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Jul 16 02:20:28 PDT 2019
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
On my system clang-format has some complaints, so I think you need to rerun clang-format. Probably caused by the rebasing.
I have some minor comments about the TestAST.py (see the inline comments), but otherwise this patch LGTM.
Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:1
+"""Test that importing modules in C works as expected."""
This comment (and the name of the test class) are copy-pasted from TestCModules and should be updated. If we can merge the two test cases (see the inline comment below), then this would be fixed as a side-effect.
Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:57
+ # This expr command imports __sFILE with definition
+ # (FILE is a typedef to __sFILE.)
I think this test has a good chance to fail once someone renamed/removes/changes this internal struct (especially if it's currently abstracted with a typedef). Would it be possible to just make a minimal module with open and FILE/__sFILE instead? If it's too much trouble, then I'm also fine with merging this as-is (as this regression is easy to fix).
Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:67
+ # Check that the AST log contains exactly one definition of __sFILE.
+ f = open(log_file)
+ log_lines = f.readlines()
It seems that this is the only different in the test compared to TestCModules.py. Would it be possible to just add this logging/checking to TestCModules.py as it's anyway testing something very similar?
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the lldb-commits