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.) + self.expect( ---------------- 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61333/new/ https://reviews.llvm.org/D61333 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
