- User Since
- Jul 12 2018, 2:31 PM (89 w, 5 d)
You also mentioned the id case failing as well. We should add that case to the test as well.
Fri, Mar 27
Fixing up loose language in the comments and adding periods.
Thu, Mar 26
- Expanded comments
- Adjusted conditionals
- Adding more detailed comments
- Adding test for cases that currently fail b/c we don't enable C++20
Wed, Mar 25
Tue, Mar 24
We need tests it looks like UriParserTest.cpp is the right place.
Mon, Mar 23
Fri, Mar 20
Adding addition tests for references
Addressing comments on naming and not duplicating the regex
Thu, Mar 19
Wed, Mar 18
Tue, Mar 17
Mon, Mar 16
Addressing minor comments
Fri, Mar 13
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.
Thu, Mar 12
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.
Incorporate feedback on how to verify the results.
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.
Wed, Mar 11
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.
Tue, Mar 10
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.
Mon, Mar 9
Moving to using ItaniumPartialDemangler for now.
LGTM WDYT @teemperor
Fri, Mar 6
Wed, Mar 4
- Converting function to static Vs anon namespace
- Updating naming
- moving assignment into if
Factored out template parameter stripping into its own function.
Tue, Mar 3
- Addressing comments
- Fixing bug, I was setting NameWithoutTemplate incorrectly in some cases.
Feb 28 2020
Feb 25 2020
Feb 24 2020
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.
Feb 21 2020
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
Feb 20 2020
Feb 14 2020
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.
Feb 13 2020
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,
If we still see the info using -R then I am happy but Jim's concerns are valid, they won't match the bracktrace.
Feb 12 2020
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.
Feb 10 2020
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!
Feb 7 2020
Feb 5 2020
Feb 4 2020
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.
Feb 3 2020
Is this missing additions to TestDataFormatterLibcxxString.py?