You also mentioned the id case failing as well. We should add that case to the test as well.

Minor fixes

Fixing up loose language in the comments and adding periods.

• Expanded comments
• Adjusted conditionals

Addressing comments:

• Adding more detailed comments
• Adding test for cases that currently fail b/c we don't enable C++20

We need tests it looks like UriParserTest.cpp is the right place.

Adding addition tests for references

Addressing comments on naming and not duplicating the regex

In D76168#1929211, @labath wrote:

In D76168#1925176, @shafik wrote:

Long-term I would like to modify clang to stop doing that for LLDB, but LLDB will still have to support older compilers for a while. So I think this fix is still needed.

So is this some alternative to D75761 (where we'd use CPlusPlusNameParser to decode DW_AT_names of templates)? If so, I think that is an interesting direction, but beware that that class is kind of meant for processing the demangler output. The contents of DW_AT_name looks a bit like a demangled name, but in reality there are some deviations from that format (which is why I did not recommend this direction initially -- but I am not against it either).

Also it looks like llvm and gnu demanglers disagree on the exact formatting of demangled operator names:

$ c++filt _ZlsI1AEvT_S1_
void operator<< <A>(A, A)
$ llvm-cxxfilt _ZlsI1AEvT_S1_
void operator<<<A>(A, A)

I think the gnu format is superior (and unambiguous) so we could change llvm to match that -- and this change probably won't require any kind of compatibility hacks.

Addressing minor comments

In D76168#1924539, @hubert.reinterpretcast wrote:

I am not sure what the usage scenario is that this is meant to support. Is it user input that tries to name a specialization of a template operator< without separation to prevent tokenization as operator<<? I think that case should qualify as user error.

Both C++17 subclause 17.2 [temp.names] and N4849 subclause 13.3 [temp.names] imbue the "angle-bracket" treatment of < to after identification of a name (not to the formation of the name itself) and does involve special treatment of <<. Special treatment in tokenization (C++17 subclause 5.4 [lex.pptoken]; N4849 subclause 5.4 [lex.pptoken]) for angle brackets extends to <: and not to <<.

All of Clang, GCC, ICC, and MSVC do not compile the following:

enum E { E0 };
template <typename T> void operator<(E, T);

void g() {
  operator<<int>(E0, 0);  // does not compile
}

Compiler Explorer (godbolt.org) >>link<<.

In other words, where is the input coming from? Should the producer of the input be corrected instead?

Addressing comments

My fix in ConsumeOperator() is not proper but if everyone feels this is correct approach I will create member functions to deal with this cleanly.

Adding potentially relevant reviews.

After discussing this @teemperor in some detail it looks like getting the error_msg case to work well is not a trivial task. So for these cases we should revert to just using expect. I think the plan is that he will remove the feature for now.

Moving to using assertIn as suggest in comment.

In D76080#1920483, @teemperor wrote:

• All of the asserts should print a useful error when failing (i.e., one that allows us to directly write a fix). You could do assertIn which is clearer than find(...) and automatically gives an error message that is useful. For the assertTrue just print the unexpected result in the assert message as the old code did.

Incorporate feedback on how to verify the results.

In D76080#1919862, @jingham wrote:

I am open to suggestions on alternative approaches, for some context I ran into this trying to add a failing test to D75761 as was suggested.

I want to see how you end up resolving the comments on payload being a plain integer type in D75626 before looking at this again. Maybe it makes more sense to use a type, the use is pretty clever but perhaps makes for opaque code in some places.

LGTM

I believe that your main example violates basic.scope.class p2:

A bunch of small comments but a few more serious ones as well.

Move to using expect_expr in the test.

In D75761#1912038, @labath wrote:

It's a pity that the clang's DW_AT_name value is so ambiguous. For example, gcc would output the name in the commit message as operator< <A::B>, which is a lot more parsable (for computers and people). Have you looked at changing clang's output to make it more like gcc's ? I know it would only help new compilers, but maybe for such a special case, this does not matter?

Or, even if we do end up adding some compat parsing code, that would reduce the need for it to be super exact. I'm pretty sure that the current code does not handle all cases correctly. E.g., I believe it will break on something like operator<<<&operator>> > (mangled name _ZlsIXadL_Zrs1AS0_EEEvS0_S0_, godbolt link).

As an alternative, we could try extracting the same information from the mangled name (via llvm::ItaniumPartialDemangler::getFunctionBaseName), at least when DW_AT_linkage_name is present (not all compilers emit that attribute but I am not sure if anyone has tested if lldb actually works without it).

Moving to using ItaniumPartialDemangler for now.

LGTM WDYT @teemperor

LGTM

Addressing comment

• Converting function to static Vs anon namespace
• Updating naming
• moving assignment into if
• clang-format

Factored out template parameter stripping into its own function.

• Addressing comments
• Fixing bug, I was setting NameWithoutTemplate incorrectly in some cases.

Nice fix!

A few comments and I would like @teemperor to give this a look as well but it looks good to me.

LGTM aside from the comments I made.

I have struggled to understand the real use of those counters, they may have been helpful to someone at one point but the rationale is lost in the sands of time.

LGTM outside of my comments

I would love to have a minimal test case for this but every approach I tried has failed, so I would appreciate if there are ideas here.

This is a great change, it makes the code more consistent. LGTM besides the few comments I made.

I am going to abandon this change b/c the consensus seems to be that we want a different solution and I don't know how much work would require ATM but I may revisit another time,

LGTM

If we still see the info using -R then I am happy but Jim's concerns are valid, they won't match the bracktrace.

I can see how stripping __1 would be nice but I seeing (anonymous namespace) may be useful to know especially b/c it effects visibility and linkage. It would be nicer if could make this optional and default it off but be able to turn it back on.

LGTM otherwise.

Updated approach based on comments and added test for the new approach.

I will try to look into dwarfdump --verify separately.

Thank you fixing this!

Since we are doing the same test all over m_addr_size >= 1 && m_addr_size <= 8 can we just make it a function and avoid the repetition and potential erroneous updating later on that does not fix them all?

I forgot to mention this earlier but LLDB is a major user of the ASTImporter and so in general it is a good idea to run check-lldb as well.

Everyone has brought up great feedback, let me go back and revise this.

In D73282#1855888, @dblaikie wrote:

I don't think the more targeted fix is a good thing - is the same problem of not demangling names not there for the other cases that the original fix addressed?

Is this missing additions to TestDataFormatterLibcxxString.py?