Changes based on comments:

• Fixing test based on comments

• Refactored test to use a function to do repetitive task

It is not obvious what the change in result is expected to be so a test would be helpful.

Addressing comments

Thank you! LGTM

In D64480#1654354, @balazske wrote:

In D64480#1653629, @shafik wrote:

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

Should we handle this case? This can be special for C99 only when the declarations must be merged instead of linked. Probably this does not cause functional problems if we leave it as is.

It is worth noting that:

LGTM but I agree w/ Gabor's comments

I was concerned about how this would affect LLDB but after thinking about it I realized that in the DWARF we will just end up with one DW_TAG_class_type.

Simplifying test.

Updating test to be more specific

Other than my two comment this LGTM

• Adding more context
• Adjusting the test to be more strict

• Adding documentation to LangRef.rst
• Simplified the test further.

@djtodoro Thank for the suggestions on how to simplify the test even more.

In D66447#1638783, @labath wrote:

In D66447#1638047, @JDevlieghere wrote:

In D66447#1637640, @labath wrote:

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

I only glanced at the proposal, but unless I misunderstand the type only fits UTF-8 characters representable in 1 byte, which are basically just ASCII.

I have now too glanced at the proposal (just the cppreference page, really :) ). I think I understand where you got this impression from, but I don't think that is fully correct. It is true that a *single* char8_t variable can hold only 8 bit UTF8 code units (*not* characters), but that is not surprising since UTF8 is a variable length encoding, so you can't have a type that matches one character exactly. However, an *array* of char8_t is a completely different thing, and I am pretty sure that these are intended to hold utf8 strings containing any utf8 characters (otherwise, it wouldn't really deserve to call itself a utf8 type), and so we should print (and test) it as regular utf8.

However, this actually surfaces the question of how should we format single char8_t variables. It makes sense to display the character value if the value happens to be ASCII, but I guess we shouldn't print something like "invalid utf8 character" if it does contain one unit of the multibyte characters.

Changed isExportSymbols -> getExportSymbols

I also just realized that although I originally talked about LLDB being the consumer, ultimately since we have to assume any AST we generate from DWARF can be used in expression parsing clang is also the consumer as well.

I am not enthusiastic about this solution but I need to think about it some more.

In D66352#1636298, @dblaikie wrote:

The difference between those two cases would be based on their usage - the types are the same (kinda, basically). In one case there's an anonymous member of the anonymous type (so it's transparent/all the nested names are as-if they were names of members in the outer type) and in the other case there's a named variable of the anonymous type (so the named members are members of that outer member variable).

In D66352#1636241, @aprantl wrote:

In D66352#1636225, @dblaikie wrote:

Eh, I think it's still pretty unnecessary (& the DWARF spec is only suggestive, doesn't tend to have requirements in this regard) and so I'd probably suggest waiting until some consumer really wants this (& I'd still want to have a discussion with the consumer about why they find this to be needed). But it's really cheap (especially in DWARFv5 where it can use a const_value form & cost no bytes in debug_info (if/when that sort of thing is implemented in LLVM, if it isn't already).

@shafik, Can you outline the problem Clang has differentiating anonymous structs and lambdas that made this patch necessary?

Deleting some lines from the test as requested.

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

I was hoping to be able reproduce this in LLDB via an expression like this:

Fixing has_name to better reflect the condition.

@aprantl @teemperor Good catches, I have updated the review.

Addressing comments:

@aprantl I did not know either until I looked at decorators.py and it says:

Adding as suggested.

@labath @stella.stamenova I updated the diff with a fix that I believe should address the test failure.

Fix that should fix the failing PDB test.

Thank you for writing this up! I just have a few minor comments.

@stella.stamenova that is unfortunate but not surprising. I don't have a way to test a fix locally. Is there anyone who might be able to help me iterate over a fix or maybe a new maintainer of the PDB parsing?

@stella.stamenova this could potentially break the windows build, could you please verify before I land this change. Thank you in advance!

Simplifying Makefile even more

Addressing comments:

• Simplifying Makefile
• Adding comments to the test

@aprantl @clayborg Thank you for the comments, I think I have addressed your concerns.

First round of review.

Thanks for fixing this one!

LGTM

Also once we get one test going then it should be easy to add coverage for all sorts of scenarios. Who knows maybe we will find more bugs.

Is it worth it to write a test that verifies the output? Otherwise LGTM.