- User Since
- Aug 26 2016, 6:53 AM (212 w, 5 d)
This was landed as 4 commits, this diff is all 4 as committed.
Address review comments.
Thanks! I've addressed most of the comments one way or the other, please LMK if it's not convincing!
Meanwhile I'm going to split this up to land in parts.
(I do wonder whether it's safe to just drop the mapping table entirely now...)
To give a bit of context here: the end-goal here is hot-reload of compile_commands.json. (This covers our *two* top bugs: #192 and #83).
Tue, Sep 22
Mon, Sep 21
Forgot to mention, 3% memory saving is huge, way more than I expected (was mostly just hoping for no regression).
A rough design sketch:
- I agree we should solve this, thanks for pushing back
- I think the path-to-error (e.g. processes.triple) is critical in some cases
- this really seems like an marshaling concern rather than something for Value/Object
- happy to take a stab at a design but I think it'll need some prototyping
Fri, Sep 18
Argh, I hit enter too soon. One thing I'd planned to add was I'm sorry about the delay in reviewing this, too.
This is a sensible thing to want and very nice code.
I'm not sure about putting it in JSON.h - there are a few unfortunate aspects that I just don't know how to fix, but limit the benefit which has to be measured against the cost of adding more complexity to the API.
Sorry about the delay, good guess as to what was happening :-)
Looks good apart from a minor technical issue with the traits.
This is really nice. There's an argument for parsing and emitting (validation and formatting) but obviously also a performance tradeoff so the feature makes sense.
I'm not sure I love having the assertion for contains-errors every place that handles dependent code in C.
We should certainly somehow document the reason dependence is possible, just as the possibility of overloads is documented.
But the assertion seems pretty heavyweight :-\
This seems like fixing a fairly specific case of a general problem: if the base class is invalid I would expect to see similar problems in other places (e.g. unqualified lookup from inside the class).
But it fixes a diagnostic the libc++ folks think is important...
Just checking this is waiting on you rather than me...
Any interest in chasing these diagnostic regressions (they seem like they might be tractable) or should we abandon this?
@hokein I think this patch is obsolete, right?
(This is obsolete now I believe, the flag has been flipped)
We do now have tests in unittests/ModulesTests.cpp that likely covers at least part of this?
@hokein Did this get landed at some point?
Just for the record, as I think Kadir and Adam are both aware...
LG from my side!
Can we add a simple test to TweakTests (I should really split up that file) and to ConfigCompileTests?
Wed, Sep 16
Just some naming and doc nits. This looks really solid now, nice job!
Thanks for catching this! Lucky coincidence it was spotted twice just before rc3!
Ugh, the hole goes deeper, because the Itanium ABI unambiguously follows the C++11 (instantiation-dependent) behavior.
Definitely need Richard's input here as I'm fairly sure this will break things as-is.
We should check if the mangling matches GCC (or if it depends on -std), if we can.
Thanks for doing this! (Agree with Haojian's comments too)
Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.
Argh, sorry. I think this might be too late to get into 11 - we're in the "soon=final" stage and maybe any changes at all cause logistical problems? :-(
Mon, Sep 14
Thu, Sep 3
Nit on the commit message: I think this is TemplateArgumentLoc 48 -> 32 bytes, and DynTypedNode 56 -> 40 bytes.
Neither of the testcases look like the right behavior to me, and I think the bug is elsewhere in clang.
Wed, Sep 2
Tue, Sep 1
Mon, Aug 31
Sorry, this fell off my radar :-(
Thu, Aug 27
Strangely, -Xclang -fno-recovery-ast-type doesn't make it go away - how sure are we that your patch is the culprit? And how sure are we that those flags work properly?
@hokein here's a minimized version:
Wed, Aug 26
Oops, nice catch!
Aug 21 2020
New diagnostics are really good :-)
I think the code could be a bit clearer, but up to you. The tests are good so feel free to submit any version you're happy with.
Aug 20 2020
Aug 19 2020
Somehow I missed the email from your replies.
Sorry, I was just not reading the test clearly. LGTM