[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

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.)
+        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