Re: Noteworthy changes when porting to C++17

it. > > But I digress ... > > So the question is: did you notice things that have been removed from > the C++ standard since C++11 that were used in our applications? -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.

Re: Notice of withdrawal of CI services: KDevelop and KDE Connect

On Donnerstag, 17. Juni 2021 11:25:15 CEST Ben Cooksley wrote: > On Thu, Jun 17, 2021 at 8:44 AM Milian Wolff wrote: > > On Mittwoch, 16. Juni 2021 20:28:25 CEST Ben Cooksley wrote: > > > Hi all, > > > > > > > > > The following is noti

Re: Notice of withdrawal of CI services: KDevelop and KDE Connect

ef checks to skip it on freebsd. Cheers and thanks for letting me know about this problem -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.

D28039: optimize dynamic regex matching

mwolff added a comment. Let me try to explain the skip offset idea (it's been years since I came up with this in GeSHi :) ) A code highlighter will repeatedly ask all highlight contexts and items therein to find the closest token to highlight next to the current cursor position. The

D27065: Fix inverted logic in IOKitStorage::isRemovable

This revision was automatically updated to reflect the committed changes. Closed by commit R245:d2172d3c0843: Fix inverted logic in IOKitStorage::isRemovable (authored by mwolff). REPOSITORY R245 Solid CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27065?vs=74772=75857 REVISION

D27285: Add left/right indent fill (as opposed to left-only), extend indent lines to broken lines

mwolff added a comment. In D27285#611163 , @dhaumann wrote: > Ok, I now fully got this, thanks for the explanation. And indeed without any left fill visual feedback is missing. > > What I wonder is how much of this needs to be configurable.

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a comment. ping? any update? REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D27065 To: mwolff, #frameworks, rjvbb, cgilles Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a comment. In D27065#608920 , @rjvbb wrote: > How do you connect? The Mac OS has a built-in VNC server but it has to be activated. Once it is you should be able to connect using any VNC client (possibly using ssh tunnelling?).

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a comment. I now got remote access to a macOS machine. That's where I now got the output from. How do I see all drives in finder? I.e. how could I make the association between the `solid-hardware5` output and finder? REPOSITORY R245 Solid REVISION DETAIL

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a comment. In D27065#608818 , @rjvbb wrote: > In D27065#608746 , @mwolff wrote: > > > before: > > > [snip] > > > Note the `Ejectable = false (bool)` vs.

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a comment. before: udi = 'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver/APPLE SSD SM0256F Media' parent =

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a reviewer: rjvbb. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D27065 To: mwolff, #frameworks, rjvbb Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D22477: With auto completion don't show completions that don't match from beginning of typed word

mwolff added a comment. I agree, this is a feature, not a bug. If people are annoyed, they are usually annoyed by other issues like the one shown by Sven REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D22477 To: ahmadsamir, #ktexteditor, cullmann, dhaumann,

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff added a reviewer: Frameworks. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D27065 To: mwolff, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

mwolff created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. mwolff requested review of this revision. REVISION SUMMARY Reading through the code, I realized that the isRemovable check returned true when the

Re: KInit - Current state and benchmarks

On Montag, 25. November 2019 22:57:11 CET Albert Astals Cid wrote: > El dissabte, 23 de novembre de 2019, a les 11:47:40 CET, Milian Wolff va escriure: > > On Mittwoch, 19. Juni 2019 19:57:56 CET Albert Astals Cid wrote: > > > El dimarts, 18 de juny de 2019, a les 12:04:38 CES

Re: KInit - Current state and benchmarks

ne compared to more modern machines? May I ask how old this machine is and what the speed of the HDD is? Thanks -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.

D24568: Provide clang-format target with a KDE Frameworks style file

mwolff added a comment. In D24568#545942 , @cullmann wrote: > In D24568#545736 , @apol wrote: > > > I'm not sure how this works, but would it be possible to have a target that only works on a patch?

D17241: WIP:Disable highlighting after 512 characters on a line.

mwolff added a comment. I personally dislike this too. INLINE COMMENTS > katerenderer.cpp:408 > +auto = renderRanges.pushNewRange(); > for (int i = 0; i < al.count(); ++i) > if (al[i].length > 0 && al[i].attributeValue > 0) { couldn't the al.count be limited

D20606: Toggle folding of child ranges by right click

mwolff added a comment. In D20606#451459 , @dhaumann wrote: > I would prefer a context menu that has this as action. This is much better discoverable and also extensible with more folding actions. I agree. Right-click should show a

D19876: Fix: apply correctly the text colors of the chosen scheme

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. one minor nit, otherwise looks like a good improvement INLINE COMMENTS > katehighlight.cpp:78 > +if (schema == QLatin1String("Normal")) { > +return

D18793: Handle text completion with block selection mode

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. we override execution in our own completion models, so this patch will only change the behavior for the builtin word and keyword completion models in ktexteditor I believe

D18612: Cache the default KColorScheme configuration

This revision was automatically updated to reflect the committed changes. Closed by commit R265:c0cc6b8a200a: Cache the default KColorScheme configuration (authored by mwolff). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D18612?vs=50541=51212#toc REPOSITORY R265 KConfigWidgets

D18612: Cache the default KColorScheme configuration

mwolff added a comment. pushed this now with a proper benchmark too, shows a ~10x performance win when a non-empty PATH is set REPOSITORY R265 KConfigWidgets BRANCH master REVISION DETAIL https://phabricator.kde.org/D18612 To: mwolff, #kate, #kdevelop, dfaure, broulik Cc: broulik,

D17932: Improvements to completion

mwolff added a comment. I'm in favor! REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17932 To: thomassc, #ktexteditor, #kdevelop, mwolff Cc: dhaumann, apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns,

D18163: Set the color scheme to Printing for Print Preview

mwolff accepted this revision. mwolff added a comment. This revision is now accepted and ready to land. yes, thanks! REPOSITORY R39 KTextEditor BRANCH print-preview-text-color (branched from master) REVISION DETAIL https://phabricator.kde.org/D18163 To: ahmadsamir, cullmann,

D18612: Cache the default KColorScheme configuration

mwolff added a comment. sure, but first let's get this in. @broulik or @dfaure care to give your +1? REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D18612 To: mwolff, #kate, #kdevelop, dfaure Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D18163: Set the color scheme to Printing when print preview'ing

mwolff requested changes to this revision. mwolff added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kateprinter.cpp:164 > QPrinter printer; > -KatePrinterPrivate p(view->doc(), view); > +KatePrinterPrivate p(view->doc(), view, true); >

D18612: Cache the default KColorScheme configuration

mwolff added a comment. I see that it's faster when I profile kate/kdev, but I cannot easily write a benchmark for this. It's only noticeable when the KDE_COLOR_SCHEME_PATH variable is set, otherwise the global application config will be used afte rall, which is going to be shared most

D18612: Cache the default KColorScheme configuration

mwolff created this revision. mwolff added reviewers: Kate, KDevelop, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. mwolff requested review of this revision. REVISION SUMMARY KDevelop, Kate and probably other applications too, recreate

D17137: KTextEditor: File menu: Put Save, Print and Export in submenus

mwolff resigned from this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17137 To: gregormi, #kate, #kdevelop Cc: loh.tar, anthonyfieroni, ngraham, cullmann, flherne, dhaumann, kwrite-devel, kde-frameworks-devel, hase, michaelh, bruns, demsking, sars

D17949: ViewPrivate: Make 'Apply Word Wrap' more comfortable

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. please look at the existing tests and expand that. There's e.g. `KateDocumentTest::testWordWrap` in `ktexteditor/autotests/src/katedocument_test.cpp` also don't change the

D17693: DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. Ok, that behavior you describe is bad. I would get annoyed by that, esp. since the list of chars that you use are totally language specific. If you want to get this behavior

D17693: DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection

mwolff added a comment. can you say what this exactly does? from reading the code and the somewhat vague commit message, it makes me believe that when I have anything selected and then press e.g. `?` the selection becomes wrapped in two `?` - is that right? when would we ever want this?

D17932: Improvements to completion

mwolff accepted this revision as: mwolff. mwolff added a comment. @brauch, @kfunk what do you say? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17932 To: thomassc, #ktexteditor, #kdevelop, mwolff Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel,

D18163: KateRenderer: when printing initially set the color scheme to Printing

mwolff accepted this revision as: mwolff. mwolff added a comment. This revision is now accepted and ready to land. lgtm, @cullmann @dhaumann? could the schema name be translated (I hope not)? REPOSITORY R39 KTextEditor BRANCH print-preview-text-color REVISION DETAIL

D17949: ViewPrivate: Make applyWordWrap() more comfortable

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. I like what I'm seeing in the screenshot, but please add proper tests for this functionality INLINE COMMENTS > kateview.cpp:2355 > +// Because we shrink and expand lines, we

D17128: WIP DocumentPrivate: Remove all from next line which may annoying when joining lines

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. can you please rephrase the title of this review to make it understandable? "Remove all" - what do you remove? All what? Also, can you please add tests for the feature you

D17932: Improvements to completion

mwolff added subscribers: brauch, kfunk, apol. mwolff added a comment. In D17932#392060 , @thomassc wrote: > Thanks for reviewing. Regarding the question about which models would have an insensitive exact match, and which ones have sensitive

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

mwolff added inline comments. INLINE COMMENTS > mwolff wrote in textdocument.cpp:691 > can't you just add a slot here that removes the actions we added once the > menu is closed? that would fix this issue with way less code and with slot I mean an local lambda that takes a copy of the actions

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

mwolff requested changes to this revision. mwolff added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > textdocument.cpp:691 > -menu->addAction(action); > -} > -} can't you just add a slot here that removes the actions we added once the menu is

D17932: Improvements to completion

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. Hey there, sorry for the long delay. In general, I think your suggestions are very sane - most notably preferring exact case matches over fuzzy matches is a good thing to have!

D17241: Disable highlighting for lines longer than 1024 characters.

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. what sven said, we should also remove the code to disable highlighting altogether when the line limit is reached, no? INLINE COMMENTS > katerenderer.cpp:387 > > +if

D13515: Remove KNS::Engine d-pointer hack

mwolff accepted this revision. mwolff added a comment. This revision is now accepted and ready to land. this is binary compatible from what I can see REPOSITORY R304 KNewStuff BRANCH master REVISION DETAIL https://phabricator.kde.org/D13515 To: apol, #frameworks, leinir, mwolff Cc:

D13365: Fixed the cursor(caret) width in kate

mwolff added a comment. @shubham looks like you are a victim of https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/ right? :D REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D13365 To: shubham, #ktexteditor, brauch, cullmann Cc:

D12756: [KDateTable] Use a more visible red color

mwolff added a comment. Let's try to fix the bug for real, instead of implementing half-baked workarounds that only work for the default configurations. Alternative ideas: add setters for the two possible cell text colors, then somehow set the KColorScheme color from the outside.

D12756: [KDateTable] Use a more visible red color

mwolff added a comment. 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. REPOSITORY R236 KWidgetsAddons REVISION DETAIL

D12511: optimization of KTextEditor::DocumentPrivate::views()

mwolff accepted this revision. mwolff added a comment. 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. But note once again the stark difference in

D12511: optimization of KTextEditor::DocumentPrivate::views()

mwolff added a comment. `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.

D12511: optimization of KTextEditor::DocumentPrivate::views()

mwolff added a comment. 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

D12511: optimization of KTextEditor::DocumentPrivate::views()

mwolff added a comment. 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.

D12511: optimization of KTextEditor::DocumentPrivate::views()

mwolff added a comment. 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. You are doing such good work with profiling and

D12511: optimization of KTextEditor::DocumentPrivate::views()

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. lgtm in general, but codewise can be improved INLINE COMMENTS > katedocument.cpp:2835 > m_views.insert(view, static_cast(view)); > +m_viewsCache = m_views.keys(); >

D12016: [ktexteditor] much faster positionFromCursor

mwolff added a comment. lgtm, @cullmann ? 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... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D12016 To: jtamate, #kate, cullmann,

D11902: Add highlighting for GDB command listings and gdbinit files

This revision was automatically updated to reflect the committed changes. Closed by commit R216:88288f834175: Add highlighting for GDB command listings and gdbinit files (authored by mwolff). REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE

D11902: Add highlighting for GDB command listings and gdbinit files

mwolff added a comment. Ping? Or can I just commit? REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D11902 To: mwolff, vkrause, dhaumann, #kate Cc: #frameworks, michaelh, ngraham, bruns

D12016: [ktexteditor] much faster positionFromCursor

mwolff added a comment. some nits, otherwise lgtm assuming all tests pass bonus points for a proper benchmark that shows the before/after gain in hard numbers INLINE COMMENTS > kateviewaccessible.h:191 > { > -int pos = 0; > -for (int line = 0; line < cursor.line();

D11811: avoid Asan runtime error: shift exponent -1 is negative

mwolff accepted this revision. mwolff added a comment. This revision is now accepted and ready to land. thanks REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11811 To: jtamate, #kate, #frameworks, mwolff Cc: mwolff, brauch, michaelh, kevinapavew, ngraham,

D11685: Implement single click on line number to select line of text

mwolff added a comment. the "also" in your commit message: can you split this commit into two parts, or is the feature addition also fixing the bug? Put differently: Could you first fix the bug, then add the feature, in separate commits? INLINE COMMENTS > kateviewhelpers.cpp:2018 >

D11811: avoid Asan runtime error: shift exponent -1 is negative

mwolff added a comment. maybe instead make it explicit what the old code did instead? i.e. check what you get from GCC for "1 << -1" and then use that when i == 0 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11811 To: jtamate, #kate, #frameworks Cc: mwolff,

D7909: Add syntax support for Crystal Programming Language

mwolff added subscribers: cullmann, mwolff. mwolff accepted this revision as: mwolff. mwolff added a comment. we got a test file, and it seems to work for a user of the Crystal language, so +1 from my side. @dhaumann, @cullmann ? REPOSITORY R40 Kate REVISION DETAIL

D11822: Make the word/char count a global preference

mwolff added subscribers: cullmann, dhaumann, mwolff. mwolff accepted this revision as: mwolff. mwolff added a comment. This revision is now accepted and ready to land. I'm not using that feature, but code-wise this lgtm - but please wait for @dhaumann or @cullmann to give their +1 too

D11487: optimization of TextLineData::attribute

mwolff added a comment. Ah, I knew it :D I suspected missing compiler optimizations from the start... But tricky indeed for you to see! But the real question would now be: what time does it take *without* this patch for you? probably it won't be more than a minute anymore. Still, this

D11902: Add highlighting for GDB command listings and gdbinit files

mwolff added reviewers: vkrause, dhaumann, Kate. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D11902 To: mwolff, vkrause, dhaumann, #kate Cc: #frameworks, michaelh, ngraham

D11902: Add highlighting for GDB command listings and gdbinit files

mwolff created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. mwolff requested review of this revision. REVISION SUMMARY Command listings start their non-output lines with (gdb) or > for multi-line commands. gdbinit

D11183: Sonnet: don't impose the default client

mwolff added inline comments. INLINE COMMENTS > cullmann wrote in loader.cpp:103 > Isn't there a == lClients.constEnd() behind that does that? ah, then this should be simplified by using `std::any_of` instead REPOSITORY R246 Sonnet REVISION DETAIL https://phabricator.kde.org/D11183 To:

D11183: Sonnet: don't impose the default client

mwolff added inline comments. INLINE COMMENTS > loader.cpp:103 > +// if it does it will be an element of lClients. > +bool unknown = std::find_if(lClients.constBegin(), > lClients.constEnd(), [backend] (const Client *client) { > +return client->name() == backend;

D11487: optimization of TextLineData::attribute

mwolff added a comment. WTF :D Your desktop CPU has clearly a better performance than my mobile CPU, no? Is AMD really so much worse here? How can that be - I don't get it :D Anyhow, I give up trying to understand this now - thanks a lot for your repeated input Jaime! REPOSITORY R39

D11487: optimization of TextLineData::attribute

mwolff added a comment. @jtamate I just checked, the function is called with the same parameters for me locally. What output do you get for this: diff --git a/src/spellcheck/spellcheck.cpp b/src/spellcheck/spellcheck.cpp index 9e04d788..50e92885 100644 ---

D11487: optimization of TextLineData::attribute

mwolff added a comment. +1 from my side, @cullmann, @dhaumann ? I'll continue to figure out why I don't see the performance issue here REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamate, #frameworks, #kate Cc: anthonyfieroni, dhaumann, mwolff,

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#232258 , @jtamate wrote: > > One question to that though: Why do you sort/lookup by `x.offset + x.length <= p`? Note how lower_bound returns the first iterator that is _not_ going to return true. > >

D11487: optimization of TextLineData::attribute

mwolff added a comment. Also, what is "@mwolf solution" - I didn't provide any code, until now: diff --git a/src/spellcheck/spellcheck.cpp b/src/spellcheck/spellcheck.cpp index 9e04d788..3a97e5c5 100644 --- a/src/spellcheck/spellcheck.cpp +++ b/src/spellcheck/spellcheck.cpp

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#231656 , @jtamate wrote: > In D11487#231522 , @mwolff wrote: > > > @jtamate looking at your screenshots, it represents closely what I see locally. Most notably,

D11487: optimization of TextLineData::attribute

mwolff added a comment. @jtamate looking at your screenshots, it represents closely what I see locally. Most notably, there are no red underlines in your screenshots which could arise due to spell checking. Thus I really wonder why you are seeing such a big hotspot there. Try perf, or

D11487: optimization of TextLineData::attribute

mwolff added inline comments. INLINE COMMENTS > anthonyfieroni wrote in katetextline.cpp:214-216 > Use operator->, it's faster than operator* and operator. > > first->offset sorry, but that's simply not true at all. Stylistic wise I agree, but an optimizing compiler will generate the same

D11487: optimization of TextLineData::attribute

mwolff added a comment. Considering spell checking is involved - can you show a screenshot for how the file looks like for you? There shouldn't be a lot of spell checking going on, or so I hope... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To:

D11487: optimization of TextLineData::attribute

mwolff added a comment. When I open your fake file in kwrite, then raise the line length limit and wait for the file to be rendered (which takes quite some time!), perf does not show any performance issues with attribute for me. It only corresponds to ~1.3% of the CPU cycles.

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#230913 , @jtamate wrote: > In D11487#230895 , @mwolff wrote: > > > Looking at `TextLineData::addAttribute`, it doesn't seem to sort the data - how can you be sure

D11487: optimization of TextLineData::attribute

mwolff added a subscriber: dhaumann. mwolff added a comment. Looking at `TextLineData::addAttribute`, it doesn't seem to sort the data - how can you be sure that the attributes list really is sorted by `offset + length`? I think this should be done manually there, or at least asserted. The

D11487: optimization of TextLineData::attribute

mwolff added a comment. In D11487#230791 , @jtamate wrote: > In D11487#230755 , @mwolff wrote: > > > yes, definitely don't roll your own lower_bound - use the STL provided one. Are you really

D11487: optimization of TextLineData::attribute

mwolff added a comment. yes, definitely don't roll your own lower_bound - use the STL provided one. Are you really compiling in release mode while measuring this? Also, I can only repeat myself in saying that you shouldn't use callgrind for performance measurements anymore, perf/hotspot

D11487: optimization of TextLineData::attribute

mwolff added a comment. can you publish the test file and how you measured it? Then I can test it with perf too, to confirm the impact. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D11487 To: jtamate, #frameworks, #kate Cc: mwolff, cullmann, michaelh,

D11282: less expensive findByUrl in KCoreDirListerCache

mwolff added a comment. I'm not into the code base, just adding some comments to ensure everything is taken into account - maybe your initial attempt was better after all... INLINE COMMENTS > kcoredirlister.cpp:852 > +if (refresh) { > +(*it).refresh(); >

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

mwolff added a comment. no real review, just want to mention that you'll still have heap allocations for every item - now once per container it is in (due to QList and QSet and sizeof the KFileItem) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate,

D11282: less expensive findByUrl in KCoreDirListerCache

mwolff added a comment. have you considered using a hash map instead? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D11282 To: jtamate, #frameworks, dfaure Cc: mwolff, michaelh

D11132: Avoid an asan runtime error

mwolff added a comment. still accepted, fell free to submit REPOSITORY R302 KIconThemes REVISION DETAIL https://phabricator.kde.org/D11132 To: jtamate, #frameworks, mwolff Cc: mwolff, apol, anthonyfieroni, michaelh

D11132: Avoid an asan runtime error

mwolff accepted this revision. mwolff added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > kiconeffect.cpp:44 > +KIconEffectPrivate() > + : effect{{}} > + , value{{}} it may be a good idea to add a comment that links to

D10712: balooctl monitor: Resume to wait for service

mwolff accepted this revision. This revision is now accepted and ready to land. REPOSITORY R293 Baloo BRANCH resume-wait (branched from master) REVISION DETAIL https://phabricator.kde.org/D10712 To: michaelh, #baloo, #frameworks, dfaure, alexeymin, mwolff Cc: mwolff, ashaposhnikov,

D10712: balooctl monitor: Resume to wait for service

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. some minor comments, otherwise lgtm INLINE COMMENTS > monitorcommand.cpp:45 > +connect(m_dbusServiceWatcher, ::serviceUnregistered, > [this]() { > +m_err <<

D10860: Do not allow to configure separator actions via context menu

This revision was automatically updated to reflect the committed changes. Closed by commit R263:67c59d7bb56e: Do not allow to configure separator actions via context menu (authored by mwolff). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D10860?vs=28108=28110#toc REPOSITORY R263

D10860: Do not allow to configure separator actions via context menu

mwolff updated this revision to Diff 28108. mwolff added a comment. rebase REPOSITORY R263 KXmlGui CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10860?vs=28106=28108 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10860 AFFECTED FILES

D9675: Don't show context menu menu if right-clicking outside

mwolff accepted this revision. This revision is now accepted and ready to land. REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D9675 To: broulik, #frameworks, dfaure, mwolff Cc: elvisangelaccio, wbauer, michaelh

D10860: Do not allow to configure separator actions via context menu

mwolff created this revision. mwolff added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. mwolff requested review of this revision. REVISION SUMMARY It's not useful to add shortcuts for separators, nor to add them

D10670: Reduce plasmashell frozen time

mwolff added a comment. ok, looking at the reasoning in the other commit: - you need to extend the commit message here - you need to provide a comment in the code the clarifies what's going on here in general, I don't see how such a comparison can be so costly - the real problem

D10670: Reduce plasmashell frozen time

mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. what? this is totally unsafe - the reinterpret_cast below shouldn't be done on anything that has event names other than the previously used one... REPOSITORY R242 Plasma

D7968: Forward QComboBox signals instead of QComboBox lineedit signals

mwolff added a comment. In D7968#206500 , @whiting wrote: > Hey all, this change breaks Kompare https://bugs.kde.org/show_bug.cgi?id=390024 which watches a KUrlRequester's textChanged signal to update a button's enabled state. We are not seeing

D9840: Tentative patch to reduce I/O overhead of plasmashell when copying files

mwolff added a comment. I guess this can be abandoned now that the real culprit was found, no? REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D9840 To: jtamate, #frameworks, dfaure Cc: mwolff, ngraham, mmustac, michaelh

D10257: KUrlMimeData: fix handling of PreferLocalUrls

mwolff added inline comments. INLINE COMMENTS > kurlmimedata.cpp:67 > +QList uris; > +const QByteArray ba = > mimeData->data(QString::fromLatin1(s_kdeUriListMime)); > +// Code from qmimedata.cpp future cleanup: remove all the `QString::fromLatin1(s_...)` in this file with a call

D10024: Add supportedSchemes feature

This revision was automatically updated to reflect the committed changes. Closed by commit R241:6ab218dba91f: Add supportedSchemes feature (authored by mwolff). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D10024?vs=25891=26632#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D10045: remote: don't create entries with empty names

This revision was automatically updated to reflect the committed changes. Closed by commit R241:4d153df7359c: remote: dont create entries with empty names (authored by mwolff). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10045?vs=26203=26633 REVISION DETAIL

  1   2   3   4   5   >