- User Since
- Apr 16 2015, 7:53 PM (190 w, 5 d)
Tue, Dec 4
what sven said, we should also remove the code to disable highlighting altogether when the line limit is reached, no?
Wed, Nov 28
Mon, Nov 19
ah I see, then your code should indeed be better!
Sun, Nov 18
Oct 31 2018
please cleanup the test slightly and then push it directly (no need for another round of review)
Oct 28 2018
the test would go into clang's test_duchain.cpp, create a method in there with the three TestFiles with the appropriate contents. Then parse them all and finally query the functions declarations and verify that the appropriate function definition is returned always.
Oct 22 2018
I assume you ran the tests (esp. for clang?) and it all keeps passing? if so, then +1
thanks a lot! do you have commit rights, or should we push this for you?
Oct 21 2018
can you also add a test for this new behavior with white space?
looking at the code, it really is missing there. You'll need a const cast but that's OK
I'd say this new code should go into FunctionDefinition::definition instead, no? I.e. check there whether decl is already a definition and if so return that directly?
very good idea
lgtm in general, three small nitpicks and I'll have a look at the broken unit test now too.
Jul 27 2018
nasty issue, thanks for fixing it
Jul 24 2018
this can now be abandoned, right? the other patch seems to supersede this?
if it makes tests pass, I'm all for it
either implement pinos variant or keep the old code and change the header name in two places
Jul 2 2018
Could it be that the issue is that on your system, libc++ and the clang compiler builtin-headers are installed in the same prefix? On Linux at least, that is usually not the case afaik. Thus we didn't run into this issue yet.
Jun 21 2018
I also believe that this is the wrong way for fixing this.
Jun 14 2018
thanks for fixing the tests! much appreciated
Jun 13 2018
no clue, I wouldn't be surprised by bugs/regressions in Qt - I've seen a lot of breakage in item/view code recently
this is binary compatible from what I can see
Jun 12 2018
lgtm too - thanks Heinz
May 29 2018
ouch, thanks a lot for the investigation. This is similar to the QStringBuilder crashes one can get... Can you simplify the line based on my suggestion? then it's a clear +2 from me, please also include the analysis and valgrind output in your commit message.
May 17 2018
fixed by using an updated libbacktrace
May 16 2018
May 8 2018
Let's try to fix the bug for real, instead of implementing half-baked workarounds that only work for the default configurations.
Maybe instead use the HighlightText QPalette color? Hardcoding red may work for the two styles you present, but I could just set the view background to red and the text becomes unusable.
May 7 2018
so what's your plan with this now? Do you actually want to move this forward? Then we need to find a good solution that works everywhere. The numbers aren't that useful on their own, as there the current implementation which is trivial and has no external dependencies is apparently still the best by a margin...
@bshah ping? will you push this?
May 4 2018
It compiles, how do I test it? Can you please also add a README to the root of this project that explains what this is, does and how to use it?
OK, cool! That clearly shows that this patch _is_ valuable: Before we have ~6% CPU cycle cost, now it's down to 1.5% (inclusively). This is a significant reduction, so I'm all for it.
perf record -g produces unusable data files, since it relies on the frame pointer which is usually not available. Use perf record --call-graph dwarf instead. https://phabricator.kde.org/file/data/w4qogv4brtxlc5p5bnwr/PHID-FILE-q62giymcptudpl5m6bt3/kwrite_perf_after_25_dwarf_caller.png shows ~1.5% in notifyAboutRangeChange (inclusively). Is that before or after your patch here?
Actually, no. Ignore what I said. The pictures you are showing are pretty meaningless. Did you run perf with --call-graph dwarf? Better look at the flamegraph and search for the function you are interested in (Kate::TextBuffer::notifyAboutRangeChange) or use the Caller/Callee view to get an aggregated view of your change.
But the hotspot screenshot clearly shows that you are spending time on optimizing things that are barely noticeable. You have optimized a function that consumes 0.3% of the CPU cycles. It now consumes only ~0.15%, at the cost of slightly higher memory consumption.
Oh and again: please start using perf/hotspot instead of callgrind. Really, the performance numbers you get from callgrind are just *instructions*! It doesn't mean "65% of CPU". It means 65% of the instructions.
lgtm in general, but codewise can be improved
May 3 2018
May 2 2018
May 1 2018
regarding the accessibility interface, I agree that it's out of the scope of this patch. I'd say let's keep it like that for now...