[Lldb-commits] [PATCH] D72953: Fix the handling of unnamed bit-fields when parsing DWARF

[Jim Ingham via lldb-commits]

> On Jan 17, 2020, at 2:24 PM, Raphael Isemann via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> teemperor added a comment.
> 
> (Just some quick comments, will review this properly during normal working hours)
> 
> Without this fix debugging Clang with LLDB is essentially impossible, so I'm in favour of landing this with as few pre-commit refactorings as possible to make backporting easier and getting this in ASAP. We probably also want to ping Hans to get this into the 10 release branch (which was created 2 days ago, so that's just a simple cherry-pick).

If you want to make back porting easier, you might not want to use expect_expr in the tests, since that would require back porting that as well as this patch?

Jim


> 
> Also you might want to check out the recently added `expect_expr` command instead of calling `expect` (see the inline comment for an example). The API currently doesn't support the whole fuzzy substr matching (which is on purpose) so you might not be able to use it everywhere (at least the current version).
> 
> 
> 
> ================
> Comment at: lldb/packages/Python/lldbsuite/test/lang/c/bitfields/TestBitfields.py:207
> +
> +        self.expect("expr (lba.a)", VARIABLES_DISPLAYED_CORRECTLY,
> +                    substrs=['unsigned int', '2'])
> ----------------
> We can do this since last week: `self.expect_expr("(lba.a)", result_type="unsigned int", result_value="2")` which is a much more safer and convenient way of doing this.
> 
> 
> ================
> Comment at: lldb/packages/Python/lldbsuite/test/lang/c/bitfields/main.c:128
> +      uint64_t HasNonTrivialToPrimitiveDestructCUnion : 1;
> +      uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1;
> +    };
> ----------------
> I would prefer generic names as otherwise Clang folks randomly see the test when they grep for usages of these variables by name in the LLVM repo.
> 
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D72953/new/
> 
> https://reviews.llvm.org/D72953
> 
> 
>